MyDeveloperDay updated this revision to Diff 187155.
MyDeveloperDay marked 10 inline comments as done.
MyDeveloperDay added a comment.
Address review comments
- change OverrideMacro to OverrideSpelling
- change FinalMacro to FinalSpelling
- fix unit tests
- show warning without fix-it if Macros are not defined
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57087/new/
https://reviews.llvm.org/D57087
Files:
clang-tidy/modernize/UseOverrideCheck.cpp
clang-tidy/modernize/UseOverrideCheck.h
docs/ReleaseNotes.rst
docs/clang-tidy/checks/modernize-use-override.rst
test/clang-tidy/modernize-use-override-with-macro.cpp
test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
Index: test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
@@ -0,0 +1,28 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN: -config="{CheckOptions: [{key: modernize-use-override.OverrideSpelling, value: 'CUSTOM_OVERRIDE'},{key: modernize-use-override.FinalSpelling, value: 'CUSTOM_FINAL'}]}" \
+// RUN: -- -std=c++11
+
+// As if the macro was not defined.
+//#define CUSTOM_OVERRIDE override
+//#define CUSTOM_FINAL override
+
+struct Base {
+ virtual ~Base() {}
+ virtual void a();
+ virtual void b();
+};
+
+struct SimpleCases : public Base {
+public:
+ virtual ~SimpleCases();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'CUSTOM_OVERRIDE' or (rarely) 'CUSTOM_FINAL' instead of 'virtual' [modernize-use-override]
+ // CHECK-FIXES: {{^}} virtual ~SimpleCases();
+
+ void a();
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'CUSTOM_OVERRIDE' or (rarely) 'CUSTOM_FINAL' [modernize-use-override]
+ // CHECK-FIXES: {{^}} void a();
+
+ virtual void b();
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'CUSTOM_OVERRIDE' or (rarely) 'CUSTOM_FINAL' instead of 'virtual' [modernize-use-override]
+ // CHECK-FIXES: {{^}} virtual void b();
+};
Index: test/clang-tidy/modernize-use-override-with-macro.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/modernize-use-override-with-macro.cpp
@@ -0,0 +1,69 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN: -config="{CheckOptions: [{key: modernize-use-override.OverrideSpelling, value: 'OVERRIDE'},{key: modernize-use-override.FinalSpelling, value: 'FINAL'}]}" \
+// RUN: -- -std=c++11
+
+#define ABSTRACT = 0
+
+#define OVERRIDE override
+#define VIRTUAL virtual
+#define NOT_VIRTUAL
+#define NOT_OVERRIDE
+
+#define MUST_USE_RESULT __attribute__((warn_unused_result))
+#define UNUSED __attribute__((unused))
+
+struct Base {
+ virtual ~Base() {}
+ virtual void a();
+ virtual void b();
+ virtual void c();
+ virtual void e() = 0;
+ virtual void f2() const = 0;
+ virtual void g() = 0;
+ virtual void j() const;
+ virtual void k() = 0;
+ virtual void l() const;
+};
+
+struct SimpleCases : public Base {
+public:
+ virtual ~SimpleCases();
+ // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+ // CHECK-FIXES: {{^}} ~SimpleCases() OVERRIDE;
+
+ void a();
+ // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'OVERRIDE' or (rarely) 'FINAL' [modernize-use-override]
+ // CHECK-FIXES: {{^}} void a() OVERRIDE;
+
+ virtual void b();
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using 'OVERRIDE' or (rarely) 'FINAL' instead of 'virtual' [modernize-use-override]
+ // CHECK-FIXES: {{^}} void b() OVERRIDE;
+
+ virtual void c();
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+ // CHECK-FIXES: {{^}} void c() OVERRIDE;
+
+ virtual void e() = 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+ // CHECK-FIXES: {{^}} void e() OVERRIDE = 0;
+
+ virtual void f2() const = 0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+ // CHECK-FIXES: {{^}} void f2() const OVERRIDE = 0;
+
+ virtual void g() ABSTRACT;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+ // CHECK-FIXES: {{^}} void g() OVERRIDE ABSTRACT;
+
+ virtual void j() const;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+ // CHECK-FIXES: {{^}} void j() const OVERRIDE;
+
+ virtual void k() OVERRIDE;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'OVERRIDE' [modernize-use-override]
+ // CHECK-FIXES: {{^}} void k() OVERRIDE;
+
+ virtual void l() const OVERRIDE;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'OVERRIDE' [modernize-use-override]
+ // CHECK-FIXES: {{^}} void l() const OVERRIDE;
+};
Index: docs/clang-tidy/checks/modernize-use-override.rst
===================================================================
--- docs/clang-tidy/checks/modernize-use-override.rst
+++ docs/clang-tidy/checks/modernize-use-override.rst
@@ -3,5 +3,38 @@
modernize-use-override
======================
+Adds ``override`` (introduced in C++11) to overridden virtual functions and
+removes ``virtual`` from those functions as it is not required.
-Use C++11's ``override`` and remove ``virtual`` where applicable.
+``virtual`` on non base class implementations was used to help indiciate to the
+user that a function was virtual. C++ compilers did not use the presence of
+this to signify an overriden function.
+
+In C++ 11 ``override`` and ``final`` keywords were introduced to allow
+overridden functions to be marked appropriately. Their presence allows
+compilers to verify that an overridden function correctly overrides a base
+class implementation.
+
+This can be useful as compilers can generate a compile time error when:
+
+ - The base class implementation function signature changes.
+ - The user has not created the override with the correct signature.
+
+Options
+-------
+
+.. option:: OverrideSpelling
+
+ Specifies a macro to use instead of ``override``. This is useful when
+ maintaining source code that also needs to compile with a pre-C++11
+ compiler.
+
+.. option:: FinalSpelling
+
+ Specifies a macro to use instead of ``final``. This is useful when
+ maintaining source code that also needs to compile with a pre-C++11
+ compiler.
+
+.. note::
+
+ For more information on the use of ``override`` see https://en.cppreference.com/w/cpp/language/override
Index: docs/ReleaseNotes.rst
===================================================================
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -98,6 +98,10 @@
`CommentUserDefiniedLiterals`, `CommentStringLiterals`,
`CommentCharacterLiterals` & `CommentNullPtrs` options.
+- The :doc:`modernize-use-override
+ <clang-tidy/checks/modernize-use-override>` now supports `OverrideSpelling`
+ and `FinalSpelling` options.
+
Improvements to include-fixer
-----------------------------
Index: clang-tidy/modernize/UseOverrideCheck.h
===================================================================
--- clang-tidy/modernize/UseOverrideCheck.h
+++ clang-tidy/modernize/UseOverrideCheck.h
@@ -18,10 +18,14 @@
/// Use C++11's `override` and remove `virtual` where applicable.
class UseOverrideCheck : public ClangTidyCheck {
public:
- UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context) {}
+ UseOverrideCheck(StringRef Name, ClangTidyContext *Context);
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+
+private:
+ const std::string OverrideSpelling;
+ const std::string FinalSpelling;
};
} // namespace modernize
Index: clang-tidy/modernize/UseOverrideCheck.cpp
===================================================================
--- clang-tidy/modernize/UseOverrideCheck.cpp
+++ clang-tidy/modernize/UseOverrideCheck.cpp
@@ -17,6 +17,16 @@
namespace tidy {
namespace modernize {
+UseOverrideCheck::UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ OverrideSpelling(Options.get("OverrideSpelling", "override")),
+ FinalSpelling(Options.get("FinalSpelling", "final")) {}
+
+void UseOverrideCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "OverrideSpelling", OverrideSpelling);
+ Options.store(Opts, "FinalSpelling", FinalSpelling);
+}
+
void UseOverrideCheck::registerMatchers(MatchFinder *Finder) {
// Only register the matcher for C++11.
if (getLangOpts().CPlusPlus11)
@@ -67,6 +77,8 @@
const auto *Method = Result.Nodes.getNodeAs<FunctionDecl>("method");
const SourceManager &Sources = *Result.SourceManager;
+ ASTContext &Context = *Result.Context;
+
assert(Method != nullptr);
if (Method->getInstantiatedFromMemberFunction() != nullptr)
Method = Method->getInstantiatedFromMemberFunction();
@@ -86,25 +98,24 @@
return; // Nothing to do.
std::string Message;
-
if (OnlyVirtualSpecified) {
- Message =
- "prefer using 'override' or (rarely) 'final' instead of 'virtual'";
+ Message = "prefer using '%0' or (rarely) '%1' instead of 'virtual'";
} else if (KeywordCount == 0) {
- Message = "annotate this function with 'override' or (rarely) 'final'";
+ Message = "annotate this function with '%0' or (rarely) '%1'";
} else {
StringRef Redundant =
- HasVirtual ? (HasOverride && HasFinal ? "'virtual' and 'override' are"
+ HasVirtual ? (HasOverride && HasFinal ? "'virtual' and '%0' are"
: "'virtual' is")
- : "'override' is";
- StringRef Correct = HasFinal ? "'final'" : "'override'";
+ : "'%0' is";
+ StringRef Correct = HasFinal ? "'%1'" : "'%0'";
Message = (llvm::Twine(Redundant) +
" redundant since the function is already declared " + Correct)
.str();
}
- DiagnosticBuilder Diag = diag(Method->getLocation(), Message);
+ auto Diag = diag(Method->getLocation(), Message)
+ << OverrideSpelling << FinalSpelling;
CharSourceRange FileRange = Lexer::makeFileCharRange(
CharSourceRange::getTokenRange(Method->getSourceRange()), Sources,
@@ -121,7 +132,7 @@
// Add 'override' on inline declarations that don't already have it.
if (!HasFinal && !HasOverride) {
SourceLocation InsertLoc;
- StringRef ReplacementText = "override ";
+ std::string ReplacementText = OverrideSpelling + " ";
SourceLocation MethodLoc = Method->getLocation();
for (Token T : Tokens) {
@@ -151,7 +162,7 @@
// end of the declaration of the function, but prefer to put it on the
// same line as the declaration if the beginning brace for the start of
// the body falls on the next line.
- ReplacementText = " override";
+ ReplacementText = " " + OverrideSpelling;
auto LastTokenIter = std::prev(Tokens.end());
// When try statement is used instead of compound statement as
// method body - insert override keyword before it.
@@ -164,14 +175,16 @@
// For declarations marked with "= 0" or "= [default|delete]", the end
// location will point until after those markings. Therefore, the override
// keyword shouldn't be inserted at the end, but before the '='.
- if (Tokens.size() > 2 && (GetText(Tokens.back(), Sources) == "0" ||
- Tokens.back().is(tok::kw_default) ||
- Tokens.back().is(tok::kw_delete)) &&
+ if (Tokens.size() > 2 &&
+ (GetText(Tokens.back(), Sources) == "0" ||
+ Tokens.back().is(tok::kw_default) ||
+ Tokens.back().is(tok::kw_delete)) &&
GetText(Tokens[Tokens.size() - 2], Sources) == "=") {
InsertLoc = Tokens[Tokens.size() - 2].getLocation();
// Check if we need to insert a space.
- if ((Tokens[Tokens.size() - 2].getFlags() & Token::LeadingSpace) == 0)
- ReplacementText = " override ";
+ if ((Tokens[Tokens.size() - 2].getFlags() & Token::LeadingSpace) == 0) {
+ ReplacementText = " " + OverrideSpelling + " ";
+ }
} else if (GetText(Tokens.back(), Sources) == "ABSTRACT") {
InsertLoc = Tokens.back().getLocation();
}
@@ -179,8 +192,15 @@
if (!InsertLoc.isValid()) {
InsertLoc = FileRange.getEnd();
- ReplacementText = " override";
+ ReplacementText = " " + OverrideSpelling;
}
+
+ // If the override macro has been specified just ensure it exists,
+ // if not don't apply a fixit but keep the warning.
+ if (OverrideSpelling != "override" &&
+ !Context.Idents.get(OverrideSpelling).hasMacroDefinition())
+ return;
+
Diag << FixItHint::CreateInsertion(InsertLoc, ReplacementText);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
