[PATCH] D57087: [clang-tidy] add OverrideMacro to modernize-use-override check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

I tend to think that a better migration strategy is to change the compiler to a C++11-compatible one first, and then turn on C++11 mode and migrate the code (possibly file-by-file or with a different granularity). But if you observe a situation where compatibility macros for C++11 constructs are actually a better way to migrate, then the proposed functionality makes sense.



================
Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:32
+    : ClangTidyCheck(Name, Context),
+      OverrideMacro(Options.get("OverrideMacro", "override")),
+      FinalMacro(Options.get("FinalMacro", "final")) {}
----------------
I'd suggest to default to an empty string and use `override` as a fallback right in the code where the diagnostic is generated.


================
Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:42-46
+  // If we use ``override``, we require at least C++11. Use a
+  // macro with pre c++11 compilers by using OverrideMacro option.
+  if ((OverrideMacro == "override" && !getLangOpts().CPlusPlus11) ||
+      !getLangOpts().CPlusPlus)
+    return;
----------------
I think, this should be left as is, because
1. clang-tidy understands C++11 and it makes sense to run it in C++11 mode to get more information out of compiler (e.g. then OVERRIDE macro will actually be defined to `override` and clang-tidy wouldn't have to jump through the hoops to detect that a method is already declared `override`).
2. I've seen folks who just `#define override` in pre-C++11 mode to make the code compile.


================
Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:97
+
+  if (!doesOverrideMacroExist(Context, OverrideMacro))
+    return;
----------------
The utility of the function is questionable. I'd drop it and replace the call with `if (!OverrideMacro.empty() && !Context.Idents.get(OverrideMacro).hasMacroDefinition()) ...`.


================
Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:121-131
+    Message = "prefer using '" + OverrideMacro + "' or (rarely) '" +
+              FinalMacro + "' instead of 'virtual'";
   } else if (KeywordCount == 0) {
-    Message = "annotate this function with 'override' or (rarely) 'final'";
+    Message = "annotate this function with '" + OverrideMacro +
+              "' or (rarely) '" + FinalMacro + "'";
   } else {
     StringRef Redundant =
----------------
How about using diagnostic arguments instead of string concatenation (`diag(Loc, "prefer using %1 or %2") << Macro1 << Macro2;`)?


================
Comment at: test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp:95
+  // CHECK-FIXES: {{^}}  MustUseResultObject k() OVERRIDE;
+};
----------------
Please add a test where the OVERRIDE macro is already present. Same for the FINAL macro.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57087/new/

https://reviews.llvm.org/D57087