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