[clang-tidy] modernize-use-override new option AllowOverrideAndFinal

Mitchell Balan · Mitchell Balan · commit df11117086fe · 2019-11-19T07:52:32.000-05:00
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. Reviewers: alexfh, djasper, JonasToth Patch by: poelmanc Subscribers: JonasToth, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D70165
diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
@@ -20,11 +20,13 @@ 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);
 }
@@ -103,7 +105,8 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
   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;
@@ -113,8 +116,9 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
     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"
+                          : "'virtual' is")
                    : "'%0' is";
     StringRef Correct = HasFinal ? "'%1'" : "'%0'";
 
@@ -211,7 +215,7 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
     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));
diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
@@ -26,6 +26,7 @@ class UseOverrideCheck : public ClangTidyCheck {
 
 private:
   const bool IgnoreDestructors;
+  const bool AllowOverrideAndFinal;
   const std::string OverrideSpelling;
   const std::string FinalSpelling;
 };
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -170,6 +170,12 @@ Improvements to clang-tidy
   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.
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
@@ -27,6 +27,14 @@ Options
 
    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
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-use-override-allow-override-and-final.cpp
@@ -0,0 +1,40 @@
+// 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;
+};
