MyDeveloperDay created this revision.
MyDeveloperDay added reviewers: alexfh, JonasToth, hokein, Eugene.Zelenko,
aaron.ballman.
MyDeveloperDay added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.
The usefulness of **modernize-use-override** can be reduced if you have to live
in an environment where you support multiple compilers, some of which sadly are
not yet fully C++11 compliant
some codebases have to use override as a macro OVERRIDE e.g.
#if defined(COMPILER_MSVC)
#define OVERRIDE override
#elif defined(__clang__)
#define OVERRIDE override
// GCC 4.7 supports explicit virtual overrides when C++11 support is enabled.
#define OVERRIDE override
#else
#define OVERRIDE
#endif
This allows code to be compiled with C++11 compliant compilers and get warnings
and errors that clang, MSVC,gcc can give, while still allowing other legacy pre
C++11 compilers to compile the code. This can be an important step towards
modernizing C++ code whilst living in a legacy codebase.
When it comes to clang tidy, the use of the **modernize-use-override** is one
of the most useful checks, but the messages reported are inaccurate for that
codebase if the standard approach is to use the macros OVERRIDE and/or FINAL.
When combined with fix-its that introduce the C++11 override keyword, they
become fatal, resulting in the modernize-use-override check being turned off to
prevent the introduction of such errors.
This revision, allows the possibility for the replacement **override **to be a
macro instead, Allowing the clang-tidy check to be run on both pre and post
C++11 code, and allowing fix-its to be applied.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D57087
Files:
clang-tidy/modernize/UseOverrideCheck.cpp
clang-tidy/modernize/UseOverrideCheck.h
docs/clang-tidy/checks/modernize-use-override.rst
test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp
Index: test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN: -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'CUSTOM_OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++98
+
+// As if the macro was not defined.
+//#define CUSTOM_OVERRIDE override
+
+struct Base {
+ virtual ~Base() {}
+ virtual void a();
+ virtual void b();
+};
+
+struct SimpleCases : public Base {
+public:
+ virtual ~SimpleCases();
+ // CHECK-FIXES: {{^}} virtual ~SimpleCases();
+
+ void a();
+ // CHECK-FIXES: {{^}} void a();
+
+ virtual void b();
+ // CHECK-FIXES: {{^}} virtual void b();
+};
Index: test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
===================================================================
--- /dev/null
+++ test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp
@@ -0,0 +1,95 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN: -config="{CheckOptions: [{key: modernize-use-nodiscard.OverrideMacro, value: 'OVERRIDE'},{key: modernize-use-nodiscard.FinalMacro, value: 'FINAL'}]}" \
+// RUN: -- -std=c++98
+
+#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 MUST_USE_RESULT MustUseResultObject {};
+
+struct IntPair {
+ int First, Second;
+};
+
+struct Base {
+ virtual ~Base() {}
+ virtual void a();
+ virtual void b();
+ virtual void c();
+ virtual void d();
+ virtual void d2();
+ virtual void e() = 0;
+ virtual void f() = 0;
+ virtual void f2() const = 0;
+ virtual void g() = 0;
+
+ virtual void j() const;
+ virtual MustUseResultObject k();
+ virtual bool l() MUST_USE_RESULT UNUSED;
+ virtual bool n() MUST_USE_RESULT UNUSED;
+
+ virtual void m();
+ virtual void m2();
+ virtual void o() __attribute__((unused));
+
+ virtual void r() &;
+ virtual void rr() &&;
+
+ virtual void cv() const volatile;
+ virtual void cv2() const volatile;
+
+ virtual void ne() noexcept(false);
+ virtual void t() throw();
+
+ virtual void il(IntPair);
+};
+
+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 f()=0;
+ // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer using
+ // CHECK-FIXES: {{^}} void f() 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 MustUseResultObject k(); // Has an implicit attribute.
+ // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: prefer using
+ // CHECK-FIXES: {{^}} MustUseResultObject k() 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,37 @@
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`` were introduced to allow overridden
+functions to be marked appropriately. There 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:: OverrideMacro
+
+ 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:: FinalMacro
+
+ 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: 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 OverrideMacro;
+ const std::string FinalMacro;
};
} // namespace modernize
Index: clang-tidy/modernize/UseOverrideCheck.cpp
===================================================================
--- clang-tidy/modernize/UseOverrideCheck.cpp
+++ clang-tidy/modernize/UseOverrideCheck.cpp
@@ -17,10 +17,35 @@
namespace tidy {
namespace modernize {
+static bool doesOverrideMacroExist(ASTContext &Context,
+ const llvm::StringRef &MacroId) {
+ // Don't check for the Macro existence if we are using ``override``.
+ if (MacroId == "override")
+ return true;
+
+ // Otherwise look up the macro name in the context to see if its defined.
+ return Context.Idents.get(MacroId).hasMacroDefinition();
+}
+
+UseOverrideCheck::UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ OverrideMacro(Options.get("OverrideMacro", "override")),
+ FinalMacro(Options.get("FinalMacro", "final")) {}
+
+void UseOverrideCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "OverrideMacro", OverrideMacro);
+ Options.store(Opts, "FinalMacro", FinalMacro);
+}
+
void UseOverrideCheck::registerMatchers(MatchFinder *Finder) {
- // Only register the matcher for C++11.
- if (getLangOpts().CPlusPlus11)
- Finder->addMatcher(cxxMethodDecl(isOverride()).bind("method"), this);
+
+ // 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;
+
+ Finder->addMatcher(cxxMethodDecl(isOverride()).bind("method"), this);
}
// Re-lex the tokens to get precise locations to insert 'override' and remove
@@ -67,6 +92,11 @@
const auto *Method = Result.Nodes.getNodeAs<FunctionDecl>("method");
const SourceManager &Sources = *Result.SourceManager;
+ ASTContext &Context = *Result.Context;
+
+ if (!doesOverrideMacroExist(Context, OverrideMacro))
+ return;
+
assert(Method != nullptr);
if (Method->getInstantiatedFromMemberFunction() != nullptr)
Method = Method->getInstantiatedFromMemberFunction();
@@ -88,16 +118,19 @@
std::string Message;
if (OnlyVirtualSpecified) {
- Message =
- "prefer using 'override' or (rarely) 'final' instead of 'virtual'";
+ 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 =
- HasVirtual ? (HasOverride && HasFinal ? "'virtual' and 'override' are"
- : "'virtual' is")
- : "'override' is";
- StringRef Correct = HasFinal ? "'final'" : "'override'";
+ HasVirtual ? (HasOverride && HasFinal
+ ? "'virtual' and '" + OverrideMacro + "' are"
+ : "'virtual' is")
+ : "'" + OverrideMacro + "' is";
+ StringRef Correct =
+ HasFinal ? "'" + FinalMacro + "'" : "'" + OverrideMacro + "'";
Message = (llvm::Twine(Redundant) +
" redundant since the function is already declared " + Correct)
@@ -121,7 +154,7 @@
// Add 'override' on inline declarations that don't already have it.
if (!HasFinal && !HasOverride) {
SourceLocation InsertLoc;
- StringRef ReplacementText = "override ";
+ std::string ReplacementText = OverrideMacro + " ";
SourceLocation MethodLoc = Method->getLocation();
for (Token T : Tokens) {
@@ -151,7 +184,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 = " " + OverrideMacro;
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 +197,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 = " " + OverrideMacro + " ";
+ }
} else if (GetText(Tokens.back(), Sources) == "ABSTRACT") {
InsertLoc = Tokens.back().getLocation();
}
@@ -179,7 +214,7 @@
if (!InsertLoc.isValid()) {
InsertLoc = FileRange.getEnd();
- ReplacementText = " override";
+ ReplacementText = " " + OverrideMacro;
}
Diag << FixItHint::CreateInsertion(InsertLoc, ReplacementText);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
