From 68f5e5456f85fbae04cdd2ddab254736b23d5e47 Mon Sep 17 00:00:00 2001 From: Paul Hoad Date: Thu, 28 Feb 2019 20:00:48 +0000 Subject: [PATCH] [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 llvm-svn: 355132 --- .../clang-tidy/modernize/UseOverrideCheck.cpp | 50 ++++++++++------ .../clang-tidy/modernize/UseOverrideCheck.h | 7 ++- clang-tools-extra/docs/ReleaseNotes.rst | 6 +- .../clang-tidy/checks/modernize-use-override.rst | 33 +++++++++- .../modernize-use-override-with-macro.cpp | 70 ++++++++++++++++++++++ ...odernize-use-override-with-no-macro-inscope.cpp | 28 +++++++++ 6 files changed, 172 insertions(+), 22 deletions(-) create mode 100644 clang-tools-extra/test/clang-tidy/modernize-use-override-with-macro.cpp create mode 100644 clang-tools-extra/test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp index cd7ed6c..2f15213 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp @@ -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 MatchFinder::MatchResult &Result) { const auto *Method = Result.Nodes.getNodeAs("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 MatchFinder::MatchResult &Result) { 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 MatchFinder::MatchResult &Result) { // 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 MatchFinder::MatchResult &Result) { // 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 MatchFinder::MatchResult &Result) { // 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); } diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h index dc6c918..976a7ae 100644 --- a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h +++ b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h @@ -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 diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 698997b..3d3edef 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -105,7 +105,7 @@ Improvements to clang-tidy - The :doc:`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 ` check have been removed. +- The :doc:`modernize-use-override + ` now supports `OverrideSpelling` + and `FinalSpelling` options. + Improvements to include-fixer ----------------------------- diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst index 54538b5..4273c6e 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize-use-override.rst @@ -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 diff --git a/clang-tools-extra/test/clang-tidy/modernize-use-override-with-macro.cpp b/clang-tools-extra/test/clang-tidy/modernize-use-override-with-macro.cpp new file mode 100644 index 0000000..ad682f1 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/modernize-use-override-with-macro.cpp @@ -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; +}; diff --git a/clang-tools-extra/test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp b/clang-tools-extra/test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp new file mode 100644 index 0000000..97b7105 --- /dev/null +++ b/clang-tools-extra/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(); +}; -- 2.7.4