[clang-tidy] add OverrideMacro to modernize-use-override check

[MyDeveloperDay]

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.

#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.

[MyDeveloperDay]

Adding release note change

[alexfh]

I tend to think that a better migration strategy is to change the compiler to a C++11-compatible one first, and then turn on C++11 mode and migrate the code (possibly file-by-file or with a different granularity). But if you observe a situation where compatibility macros for C++11 constructs are actually a better way to migrate, then the proposed functionality makes sense.

clang-tidy/modernize/UseOverrideCheck.cpp

32

I'd suggest to default to an empty string and use override as a fallback right in the code where the diagnostic is generated.

42–46

I think, this should be left as is, because

1. clang-tidy understands C++11 and it makes sense to run it in C++11 mode to get more information out of compiler (e.g. then OVERRIDE macro will actually be defined to override and clang-tidy wouldn't have to jump through the hoops to detect that a method is already declared override).
2. I've seen folks who just #define override in pre-C++11 mode to make the code compile.

97

The utility of the function is questionable. I'd drop it and replace the call with if (!OverrideMacro.empty() && !Context.Idents.get(OverrideMacro).hasMacroDefinition()) ....

121–131

How about using diagnostic arguments instead of string concatenation (diag(Loc, "prefer using %1 or %2") << Macro1 << Macro2;)?

test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp

95

Please add a test where the OVERRIDE macro is already present. Same for the FINAL macro.

[JonasToth]

clang-tidy/modernize/UseOverrideCheck.cpp

133

Dangling? These values seem to be temporary and StringRef would bind to the temporary, not?
For the concatenation llvm::Twine would be better as well, same in the other places.

[MyDeveloperDay]

In D57087#1367610, @alexfh wrote:

I tend to think that a better migration strategy is to change the compiler to a C++11-compatible one first, and then turn on C++11 mode and migrate the code (possibly file-by-file or with a different granularity). But if you observe a situation where compatibility macros for C++11 constructs are actually a better way to migrate, then the proposed functionality makes sense.

@alexfh, I couldn't agree more, however unfortunately if you are having to support a common code base which also supports older platforms like HPUX, Solaris, AIX even getting a C++11 compiler can be more of a challenge! Those platforms have expensive commercial compilers but are often lacking behind the standards. If your lucky you can find a gcc that will work but it tends to be a low version one. And then building a later gcc on those platforms is often a challenge too, even if you can get it and your code to build you often meet other hard to find issues with the final binary.

This is similar to the discussion about getting clang to compile with a minimum C++14 or C++17 compiler, it can be hard to pull the toolchain up to the level where all the supported platforms still work.

Having said all this I appreciate the code review comments, let me take a look at rewriting the bits you highlighted...which I totally agree with and think it makes for a cleaner solution. (I was trying not alter the code too much around line 120)

[MyDeveloperDay]

Addressing review comments,

• reduce changes causing excessive string concats and problems with temporaries
• simplify cxx version and macro checks

clang-tidy/modernize/UseOverrideCheck.cpp

32

So I tried this and and met with some issues with the unit tests where it seemed to think "override" was a macro, I also found myself just simply always setting OverrideMacro/Final Macro to "override" and "final" anyway.. I've changed this round a little to only check for the macro when the OverrideMacro isn't override. This seems to resolve the problem, let me know if it still feels wrong.

42–46

I agree with this..I don't need to run clang-tidy in cxx98 mode..reverting to the original check

97

removed the function, but did change the condition here to be OverrideMacro!="override" to overcome issue in the unit test when not overriding the macro it would sometimes return here and not perform the fix-it, I wondered if check_clang_tidy.py -fix somehow ran the clang tidy check() function twice, once for the message and once for the fix.. but I didn't dig in any further.

121–131

switched to using %0 and %1 hope that is correct, that 0 vs 1 stumped me for a bit, but I looked at other checks doing the same thing

133

removed the need for so may concatenations, I hope by using %0 and %1

test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp

95

added a couple more..let me know if its sufficient.. changed the name of the test to remove cxx98 too

[alexfh]

clang-tidy/modernize/UseOverrideCheck.cpp

32

In case "override" is not a macro, setting OverrideMacro to override would be somewhat confusing. We could make set default to override, if this makes logic simpler, but then I'd suggest to rename the option to OverrideSpelling (same for final).

docs/clang-tidy/checks/modernize-use-override.rst

9–39

Please enclose "virtual" in double backquotes.

13

override and final keywords were introduced ...

14

s/There/Their/

39

Enclose override in double backquotes.

test/clang-tidy/modernize-use-override-with-macro.cpp

2 ↗

(On Diff #183268)

"modernize-use-nodiscard"? Does this test pass?

test/clang-tidy/modernize-use-override-with-no-macro-inscope.cpp

2 ↗

(On Diff #183268)

Same here.

16 ↗

(On Diff #183268)

The check should still issue a warning here, imo. Maybe without a fix, but it should draw attention to the issue.

[MyDeveloperDay]

Address review comments

• change OverrideMacro to OverrideSpelling
• change FinalMacro to FinalSpelling
• fix unit tests
• show warning without fix-it if Macros are not defined

clang-tidy/modernize/UseOverrideCheck.cpp

32

I agree that is a better naming

test/clang-tidy/modernize-use-override-with-macro.cpp

2 ↗

(On Diff #183268)

my bad I was having problems with getting lit to work in windows environment, so was running the tests externally, I've fixed that now

c:/clang/llvm/build/RelWithDebInfo/bin/llvm-lit.py -v test/clang-tidy/modernize-use-override*
-- Testing: 5 tests, 5 threads --
PASS: Clang Tools :: clang-tidy/modernize-use-override-cxx98.cpp (1 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override-ms.cpp (2 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override-with-no-macro-inscope.cpp (3 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override.cpp (4 of 5)
PASS: Clang Tools :: clang-tidy/modernize-use-override-with-macro.cpp (5 of 5)
Testing Time: 5.13s
  Expected Passes    : 5

[malcolm.parsons]

Some Clang warnings use PP.getLastMacroWithSpelling() to determine the user's macro automatically.

[MyDeveloperDay]

In D57087#1400578, @malcolm.parsons wrote:

Some Clang warnings use PP.getLastMacroWithSpelling() to determine the user's macro automatically.

This would be a neat trick, but mainly I think users would either be using override/final or they would know what they were using say OVERRIDE/FINAL because it might be set by a convention for a code base, so maybe its better if they are explicit in their tidy options

[alexfh]

LG. Sorry for the delay.

[lewmpk]

This comment was removed by lewmpk.

[MyDeveloperDay]

rebase after D58731: [clang-tidy] added cppcoreguidelines-explicit-virtual-functions change

as this was previous accepted I will land shortly, but I'd appreciate a quick overview... @JonasToth, @lewmpk

[JonasToth]

LGTM

clang-tidy/modernize/UseOverrideCheck.cpp

209

Nit: braces

clang-tidy/modernize/UseOverrideCheck.h

 
 namespace clang {
 namespace tidy {
 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) {}
+  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
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEOVERRIDECHECK_H

clang-tidy/modernize/UseOverrideCheck.cpp

 #include "clang/Lex/Lexer.h"
 
 using namespace clang::ast_matchers;
 
 namespace clang {
 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;
+
 }
 
 // Re-lex the tokens to get precise locations to insert 'override' and remove
 // 'virtual'.
 static SmallVector<Token, 16>
 ParseTokens(CharSourceRange Range, const MatchFinder::MatchResult &Result) {
   const SourceManager &Sources = *Result.SourceManager;
   std::pair<FileID, unsigned> LocInfo =
static StringRef GetText(const Token &Tok, const SourceManager &Sources) {
   return StringRef(Sources.getCharacterData(Tok.getLocation()),
                    Tok.getLength());
 }
 
 void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
   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();
 
   if (Method->isImplicit() || Method->getLocation().isMacroID() ||
       Method->isOutOfLine())
     return;
 
   bool HasVirtual = Method->isVirtualAsWritten();
   bool HasOverride = Method->getAttr<OverrideAttr>();
   bool HasFinal = Method->getAttr<FinalAttr>();
 
   bool OnlyVirtualSpecified = HasVirtual && !HasOverride && !HasFinal;
   unsigned KeywordCount = HasVirtual + HasOverride + HasFinal;
 
   if (!OnlyVirtualSpecified && KeywordCount == 1)
     return; // Nothing to do.
 
   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"
+                   : "'" + OverrideMacro + "' is";
+    StringRef Correct =
+        HasFinal ? "'" + FinalMacro + "'" : "'" + OverrideMacro + "'";
 
     Message = (llvm::Twine(Redundant) +
                " redundant since the function is already declared " + Correct)
                   .str();
   }
 
   DiagnosticBuilder Diag = diag(Method->getLocation(), Message);
 
   CharSourceRange FileRange = Lexer::makeFileCharRange(
       CharSourceRange::getTokenRange(Method->getSourceRange()), Sources,
       getLangOpts());
 
   if (!FileRange.isValid())
     return;
 
   // FIXME: Instead of re-lexing and looking for specific macros such as
   // 'ABSTRACT', properly store the location of 'virtual' and '= 0' in each
   // FunctionDecl.
   SmallVector<Token, 16> Tokens = ParseTokens(FileRange, Result);
 
   // 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) {
       if (T.is(tok::kw___attribute) &&
           !Sources.isBeforeInTranslationUnit(T.getLocation(), MethodLoc)) {
         InsertLoc = T.getLocation();
         break;
       }
if (!HasFinal && !HasOverride) {
     }
 
     if (InsertLoc.isInvalid() && Method->doesThisDeclarationHaveABody() &&
         Method->getBody() && !Method->isDefaulted()) {
       // For methods with inline definition, add the override keyword at the
       // 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.
       if (LastTokenIter->is(tok::kw_try))
         LastTokenIter = std::prev(LastTokenIter);
       InsertLoc = LastTokenIter->getEndLoc();
     }
 
     if (!InsertLoc.isValid()) {
       // 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" ||
           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();
       }
     }
 
     if (!InsertLoc.isValid()) {
       InsertLoc = FileRange.getEnd();
-      ReplacementText = " override";
+      ReplacementText = " " + OverrideMacro;
     }
     Diag << FixItHint::CreateInsertion(InsertLoc, ReplacementText);
   }
 
   if (HasFinal && HasOverride) {
     SourceLocation OverrideLoc = Method->getAttr<OverrideAttr>()->getLocation();
     Diag << FixItHint::CreateRemoval(
         CharSourceRange::getTokenRange(OverrideLoc, OverrideLoc));

docs/ReleaseNotes.rst

 --------------------------
 
 - New :doc:`abseil-duration-conversion-cast
   <clang-tidy/checks/abseil-duration-conversion-cast>` check.
 
   Checks for casts of ``absl::Duration`` conversion functions, and recommends
   the right conversion function instead.
 
+- The :doc:`modernize-use-override
+  <clang-tidy/checks/modernize-use-override>` now supports `OverrideMacro`
+  and `FinalMacro` options.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

docs/clang-tidy/checks/modernize-use-override.rst

 .. title:: clang-tidy - modernize-use-override
 
 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

test/clang-tidy/modernize-use-override-cxx98-with-macro.cpp

+// 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;
+};

test/clang-tidy/modernize-use-override-cxx98-with-no-macro-inscope.cpp

+// 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();
+};