clang-tidy: modernize-use-override new option AllowOverrideAndFinal

[poelmanc]

Summary

In addition to adding override wherever possible, clang-tidy's modernize-use-override nicely removes virtual when override or final is specified, and further removes override when final is specified. While this is great default behavior, when code needs to be compiled with gcc at high warning levels that include gcc -Wsuggest-override or gcc -Werror=suggest-override, clang-tidy's removal of the redundant override keyword causes gcc to emit a warning or error. This discrepancy / conflict has been noted by others including a comment on Stack Overflow and by Mozilla's Firefox developers.

This patch adds an AllowOverrideAndFinal option defaulting to 0 - thus preserving current behavior - that when enabled allows both override and final to co-exist, while still fixing all other issues.

The patch includes a test file verifying all combinations of virtual/override/final, and mentions the new option in the release notes.

[JonasToth]

LGTM!
Did you check on a real code-base that suffers from the issue, if that works as expected?

[poelmanc]

In D70165#1744007, @JonasToth wrote:

LGTM!
Did you check on a real code-base that suffers from the issue, if that works as expected?

Thanks! I have now run it on our real code base and it worked as expected.

I lack commit access and would appreciate anyone committing this on my behalf.

[alexfh]

While I have no objections against this patch, I wonder whether someone had a chance to ask GCC developers about this? Is it a conscious choice to suggest override when final is present? What's the argument for doing so?

[poelmanc]

In D70165#1745530, @alexfh wrote:

While I have no objections against this patch, I wonder whether someone had a chance to ask GCC developers about this? Is it a conscious choice to suggest override when final is present? What's the argument for doing so?

Thanks, someone should ask them as I believe this issue extends beyond clang-tidy: code with functions marked final cannot satisfy both gcc -Wsuggest-override and clang -Winconsistent-missing-override; gcc demands override final and clang demands just final.

Even if clang and gcc find a common ground, people will be stuck with current versions for quite a while, so this clang-tidy patch should prove helpful.

[lebedev.ri]

@poelmanc @mitchell-stellar
I'm confused by the diff - did this land? Was the correct patch committed?

[lebedev.ri]

Ah, so it *really* landed in rG06f3dabe4a2e85a32ade27c0769b6084c828a206.

[mitchell-stellar]

Yes, there was a failing unit test that had to be fixed. I reverted the first commit and then committed a version that fixed the failing test.

[lebedev.ri]

In D70165#1750606, @mitchell-stellar wrote:

Yes, there was a failing unit test that had to be fixed. I reverted the first commit and then committed a version that fixed the failing test.

I mean, the commit message (including differential link) was wrong.

[mitchell-stellar]

Oops, it looks like I mixed up this ticket with https://reviews.llvm.org/D69238. Sorry about that. Would you like me to revert both commits and then re-commit with the correct links, etc.?

[lebedev.ri]

In D70165#1751402, @mitchell-stellar wrote:

Oops, it looks like I mixed up this ticket with https://reviews.llvm.org/D69238. Sorry about that. Would you like me to revert both commits and then re-commit with the correct links, etc.?

No strong opinion, might be good to do (and reopen the differentials so they are closed with proper diffs)

[mitchell-stellar]

Reopening in order to correct the accidentally swapped commits between this ticket and https://reviews.llvm.org/D69238.

clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h

public:
   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 bool AllowOverrideAndFinal;
   const std::string OverrideSpelling;
   const std::string FinalSpelling;
 };
 
 } // namespace modernize
 } // namespace tidy
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_USEOVERRIDECHECK_H

clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp

 
 namespace clang {
 namespace tidy {
 namespace modernize {
 
 UseOverrideCheck::UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
     : ClangTidyCheck(Name, Context),
       IgnoreDestructors(Options.get("IgnoreDestructors", false)),
+      AllowOverrideAndFinal(Options.get("AllowOverrideAndFinal", 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, "AllowOverrideAndFinal", AllowOverrideAndFinal);
   Options.store(Opts, "OverrideSpelling", OverrideSpelling);
   Options.store(Opts, "FinalSpelling", FinalSpelling);
 }
 
 void UseOverrideCheck::registerMatchers(MatchFinder *Finder) {
   // Only register the matcher for C++11.
   if (!getLangOpts().CPlusPlus11)
     return;
void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
 
   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)
+  if ((!OnlyVirtualSpecified && KeywordCount == 1) ||
+      (!HasVirtual && HasOverride && HasFinal && AllowOverrideAndFinal))
     return; // Nothing to do.
 
   std::string Message;
   if (OnlyVirtualSpecified) {
     Message = "prefer using '%0' or (rarely) '%1' instead of 'virtual'";
   } else if (KeywordCount == 0) {
     Message = "annotate this function with '%0' or (rarely) '%1'";
   } else {
     StringRef Redundant =
-        HasVirtual ? (HasOverride && HasFinal ? "'virtual' and '%0' are"
-                                              : "'virtual' is")
+        HasVirtual ? (HasOverride && HasFinal && !AllowOverrideAndFinal
+                          ? "'virtual' and '%0' are"
                    : "'%0' is";
     StringRef Correct = HasFinal ? "'%1'" : "'%0'";
 
     Message = (llvm::Twine(Redundant) +
                " redundant since the function is already declared " + Correct)
                   .str();
   }
 
if (!HasFinal && !HasOverride) {
     // 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);
   }
 
-  if (HasFinal && HasOverride) {
+  if (HasFinal && HasOverride && !AllowOverrideAndFinal) {
     SourceLocation OverrideLoc = Method->getAttr<OverrideAttr>()->getLocation();
     Diag << FixItHint::CreateRemoval(
         CharSourceRange::getTokenRange(OverrideLoc, OverrideLoc));
   }
 
   if (HasVirtual) {
     for (Token Tok : Tokens) {
       if (Tok.is(tok::kw_virtual)) {

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`readability-redundant-access-specifiers
   access specifiers.
 
 - New :doc:`readability-make-member-function-const
   <clang-tidy/checks/readability-make-member-function-const>` check.
 
   Finds non-static member functions that can be made ``const``
   because the functions don't use ``this`` in a non-const way.
 
+- Improved :doc:`modernize-use-override
+  <clang-tidy/checks/modernize-use-override>` check.
+
+  The check now supports the ``AllowOverrideAndFinal`` option to eliminate
+  conflicts with ``gcc -Wsuggest-override`` or ``gcc -Werror=suggest-override``.
+
 - The :doc:`readability-redundant-string-init
   <clang-tidy/checks/readability-redundant-string-init>` check now supports a
   `StringNames` option enabling its application to custom string classes.
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...

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

 
 Options
 -------
 
 .. option:: IgnoreDestructors
 
    If set to non-zero, this check will not diagnose destructors. Default is `0`.
 
+.. option:: AllowOverrideAndFinal
+
+   If set to non-zero, this check will not diagnose ``override`` as redundant
+   with ``final``. This is useful when code will be compiled by a compiler with
+   warning/error checking flags requiring ``override`` explicitly on overriden
+   members, such as ``gcc -Wsuggest-override``/``gcc -Werror=suggest-override``.
+   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

clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp

+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: [{key: modernize-use-override.AllowOverrideAndFinal, value: 1}]}"
+
+struct Base {
+  virtual ~Base();
+  virtual void a();
+  virtual void b();
+  virtual void c();
+  virtual void d();
+  virtual void e();
+  virtual void f();
+  virtual void g();
+  virtual void h();
+  virtual void i();
+};
+
+struct Simple : public Base {
+  virtual ~Simple();
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: prefer using 'override' or (rarely) 'final' instead of 'virtual' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  ~Simple() override;
+  virtual void a() override;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'override' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void a() override;
+  virtual void b() final;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void b() final;
+  virtual void c() final override;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void c() final override;
+  virtual void d() override final;
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: 'virtual' is redundant since the function is already declared 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void d() override final;
+  void e() final override;
+  void f() override final;
+  void g() final;
+  void h() override;
+  void i();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate this function with 'override' or (rarely) 'final' [modernize-use-override]
+  // CHECK-FIXES: {{^}}  void i() override;
+};