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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
