[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