[clang-tools-extra] r355132 - [clang-tidy] add OverrideMacro to modernize-use-override check
Paul Hoad via cfe-commits
cfe-commits at lists.llvm.org
Thu Feb 28 12:00:48 PST 2019
Author: paulhoad
Date: Thu Feb 28 12:00:48 2019
New Revision: 355132
URL: http://llvm.org/viewvc/llvm-project?rev=355132&view=rev
Log:
[clang-tidy] add OverrideMacro to modernize-use-override check
Summary:
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.
```
// GCC 4.7 supports explicit virtual overrides when C++11 support is enabled.
```
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.
Reviewers: alexfh, JonasToth, hokein, Eugene.Zelenko, aaron.ballman
Reviewed By: alexfh, JonasToth
Subscribers: lewmpk, malcolm.parsons, jdoerfert, xazax.hun, cfe-commits, llvm-commits
Tags: #clang-tools-extra
Differential Revision: https://reviews.llvm.org/D57087
Added:
clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-with-macro.cpp
clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.h
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-override.rst
Modified: clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp?rev=355132&r1=355131&r2=355132&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.cpp Thu Feb 28 12:00:48 2019
@@ -17,8 +17,16 @@ namespace clang {
namespace tidy {
namespace modernize {
+UseOverrideCheck::UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context),
+ IgnoreDestructors(Options.get("IgnoreDestructors", false)),
+ OverrideSpelling(Options.get("OverrideSpelling", "override")),
+ FinalSpelling(Options.get("FinalSpelling", "final")) {}
+
void UseOverrideCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "IgnoreDestructors", IgnoreDestructors);
+ Options.store(Opts, "OverrideSpelling", OverrideSpelling);
+ Options.store(Opts, "FinalSpelling", FinalSpelling);
}
void UseOverrideCheck::registerMatchers(MatchFinder *Finder) {
@@ -78,6 +86,8 @@ void UseOverrideCheck::check(const Match
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();
@@ -97,25 +107,24 @@ void UseOverrideCheck::check(const Match
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,
@@ -132,7 +141,7 @@ void UseOverrideCheck::check(const Match
// 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) {
@@ -162,7 +171,7 @@ void UseOverrideCheck::check(const Match
// 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.
@@ -175,23 +184,30 @@ void UseOverrideCheck::check(const Match
// 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 ";
- } else if (GetText(Tokens.back(), Sources) == "ABSTRACT") {
+ ReplacementText = " " + OverrideSpelling + " ";
+ } else if (GetText(Tokens.back(), Sources) == "ABSTRACT")
InsertLoc = Tokens.back().getLocation();
- }
}
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);
}
Modified: clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.h?rev=355132&r1=355131&r2=355132&view=diff
==============================================================================
--- clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.h (original)
+++ clang-tools-extra/trunk/clang-tidy/modernize/UseOverrideCheck.h Thu Feb 28 12:00:48 2019
@@ -18,15 +18,16 @@ namespace modernize {
/// Use C++11's `override` and remove `virtual` where applicable.
class UseOverrideCheck : public ClangTidyCheck {
public:
- UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
- : ClangTidyCheck(Name, Context),
- IgnoreDestructors(Options.get("IgnoreDestructors", false)) {}
+ 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 bool IgnoreDestructors;
+ const std::string OverrideSpelling;
+ const std::string FinalSpelling;
};
} // namespace modernize
Modified: clang-tools-extra/trunk/docs/ReleaseNotes.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/ReleaseNotes.rst?rev=355132&r1=355131&r2=355132&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/ReleaseNotes.rst (original)
+++ clang-tools-extra/trunk/docs/ReleaseNotes.rst Thu Feb 28 12:00:48 2019
@@ -105,7 +105,7 @@ Improvements to clang-tidy
- The :doc:`bugprone-argument-comment
<clang-tidy/checks/bugprone-argument-comment>` now supports
- `CommentBoolLiterals`, `CommentIntegerLiterals`, `CommentFloatLiterals`,
+ `CommentBoolLiterals`, `CommentIntegerLiterals`, `CommentFloatLiterals`,
`CommentUserDefiniedLiterals`, `CommentStringLiterals`,
`CommentCharacterLiterals` & `CommentNullPtrs` options.
@@ -113,6 +113,10 @@ Improvements to clang-tidy
:doc:`objc-property-declaration <clang-tidy/checks/objc-property-declaration>`
check have been removed.
+- The :doc:`modernize-use-override
+ <clang-tidy/checks/modernize-use-override>` now supports `OverrideSpelling`
+ and `FinalSpelling` options.
+
Improvements to include-fixer
-----------------------------
Modified: clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-override.rst
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-override.rst?rev=355132&r1=355131&r2=355132&view=diff
==============================================================================
--- clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-override.rst (original)
+++ clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-use-override.rst Thu Feb 28 12:00:48 2019
@@ -3,7 +3,22 @@
modernize-use-override
======================
-Use C++11's ``override`` and remove ``virtual`` where applicable.
+Adds ``override`` (introduced in C++11) to overridden virtual functions and
+removes ``virtual`` from those functions as it is not required.
+
+``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
-------
@@ -11,3 +26,19 @@ Options
.. option:: IgnoreDestructors
If set to non-zero, this check will not diagnose destructors. Default is `0`.
+
+.. 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
Added: clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-with-macro.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-with-macro.cpp?rev=355132&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-with-macro.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-with-macro.cpp Thu Feb 28 12:00:48 2019
@@ -0,0 +1,70 @@
+// 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 FINAL final
+#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;
+};
Added: clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp?rev=355132&view=auto
==============================================================================
--- clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp (added)
+++ clang-tools-extra/trunk/test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp Thu Feb 28 12:00:48 2019
@@ -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();
+};
More information about the cfe-commits
mailing list