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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.


================
Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:32
+    : ClangTidyCheck(Name, Context),
+      OverrideMacro(Options.get("OverrideMacro", "override")),
+      FinalMacro(Options.get("FinalMacro", "final")) {}
----------------
alexfh wrote:
> I'd suggest to default to an empty string and use `override` as a fallback right in the code where the diagnostic is generated.
So I tried this and and met with some issues with the unit tests where it seemed to think "override" was a macro, I also found myself just simply always setting OverrideMacro/Final Macro to "override" and "final" anyway.. I've changed this round a little to only check for the macro when the OverrideMacro isn't override. This seems to resolve the problem, let me know if it still feels wrong.


================
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;
----------------
alexfh wrote:
> 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.
I agree with this..I don't need to run clang-tidy in cxx98 mode..reverting to the original check


================
Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:97
+
+  if (!doesOverrideMacroExist(Context, OverrideMacro))
+    return;
----------------
alexfh wrote:
> The utility of the function is questionable. I'd drop it and replace the call with `if (!OverrideMacro.empty() && !Context.Idents.get(OverrideMacro).hasMacroDefinition()) ...`.
removed the function, but did change the condition here to be OverrideMacro!="override" to overcome issue in the unit test when not overriding the macro it would sometimes return here and not perform the fix-it, I wondered if  check_clang_tidy.py -fix somehow ran the clang tidy check() function twice, once for the message and once for the fix.. but I didn't dig in any further.


================
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 =
----------------
alexfh wrote:
> How about using diagnostic arguments instead of string concatenation (`diag(Loc, "prefer using %1 or %2") << Macro1 << Macro2;`)?
switched to using %0 and %1 hope that is correct, that 0 vs 1 stumped me for a bit, but I looked at other checks doing the same thing


================
Comment at: clang-tidy/modernize/UseOverrideCheck.cpp:133
+    StringRef Correct =
+        HasFinal ? "'" + FinalMacro + "'" : "'" + OverrideMacro + "'";
 
----------------
JonasToth wrote:
> Dangling? These values seem to be temporary and `StringRef` would bind to the temporary, not?
> For the concatenation `llvm::Twine` would be better as well, same in the other places.
removed the need for so may concatenations, I hope by using %0 and %1


================
Comment at: test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp:95
+  // CHECK-FIXES: {{^}}  MustUseResultObject k() OVERRIDE;
+};
----------------
alexfh wrote:
> Please add a test where the OVERRIDE macro is already present. Same for the FINAL macro.
added a couple more..let me know if its sufficient.. changed the name of the test to remove cxx98 too


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

https://reviews.llvm.org/D57087