[clang-tools-extra] [clang-tidy] Add option to keep virtual in 'modernize-use-override' (PR #144916)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tools-extra

Author: Frank Winklmeier (fwinkl)

<details>
<summary>Changes</summary>

Add a new `AllowVirtual` option to 'modernize-use-override', which does not remove the `virtual` specifier. This is useful because some coding guidelines may suggest to use `virtual ... override` for clarity. See the new unit test for examples.


---
Full diff: https://github.com/llvm/llvm-project/pull/144916.diff


4 Files Affected:

- (modified) clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp (+10-3) 
- (modified) clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h (+1) 
- (modified) clang-tools-extra/docs/clang-tidy/checks/modernize/use-override.rst (+7-1) 
- (added) clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-allow-virtual.cpp (+25) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
index fd5bd9f0b181b..c18bd68a99b37 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.cpp
@@ -22,6 +22,7 @@ UseOverrideCheck::UseOverrideCheck(StringRef Name, ClangTidyContext *Context)
       IgnoreTemplateInstantiations(
           Options.get("IgnoreTemplateInstantiations", false)),
       AllowOverrideAndFinal(Options.get("AllowOverrideAndFinal", false)),
+      AllowVirtual(Options.get("AllowVirtual", false)),
       OverrideSpelling(Options.get("OverrideSpelling", "override")),
       FinalSpelling(Options.get("FinalSpelling", "final")) {}
 
@@ -30,6 +31,7 @@ void UseOverrideCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "IgnoreTemplateInstantiations",
                 IgnoreTemplateInstantiations);
   Options.store(Opts, "AllowOverrideAndFinal", AllowOverrideAndFinal);
+  Options.store(Opts, "AllowVirtual", AllowVirtual);
   Options.store(Opts, "OverrideSpelling", OverrideSpelling);
   Options.store(Opts, "FinalSpelling", FinalSpelling);
 }
@@ -111,15 +113,19 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
   unsigned KeywordCount = HasVirtual + HasOverride + HasFinal;
 
   if ((!OnlyVirtualSpecified && KeywordCount == 1) ||
+      (HasVirtual && HasOverride && AllowVirtual) ||
       (!HasVirtual && HasOverride && HasFinal && AllowOverrideAndFinal))
     return; // Nothing to do.
 
   std::string Message;
   if (OnlyVirtualSpecified) {
-    Message = "prefer using '%0' or (rarely) '%1' instead of 'virtual'";
+    if (AllowVirtual)
+      Message = "add '%0' or (rarely) '%1' for 'virtual' function";
+    else
+      Message = "prefer using '%0' or (rarely) '%1' instead of 'virtual'";
   } else if (KeywordCount == 0) {
     Message = "annotate this function with '%0' or (rarely) '%1'";
-  } else {
+  } else if (!AllowVirtual) {
     StringRef Redundant =
         HasVirtual ? (HasOverride && HasFinal && !AllowOverrideAndFinal
                           ? "'virtual' and '%0' are"
@@ -226,7 +232,8 @@ void UseOverrideCheck::check(const MatchFinder::MatchResult &Result) {
         CharSourceRange::getTokenRange(OverrideLoc, OverrideLoc));
   }
 
-  if (HasVirtual) {
+  // Remove virtual unless requested otherwise
+  if (HasVirtual && !AllowVirtual) {
     for (Token Tok : Tokens) {
       if (Tok.is(tok::kw_virtual)) {
         std::optional<Token> NextToken =
diff --git a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
index 2c624f48fcc85..6879221ab4284 100644
--- a/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
+++ b/clang-tools-extra/clang-tidy/modernize/UseOverrideCheck.h
@@ -29,6 +29,7 @@ class UseOverrideCheck : public ClangTidyCheck {
   const bool IgnoreDestructors;
   const bool IgnoreTemplateInstantiations;
   const bool AllowOverrideAndFinal;
+  const bool AllowVirtual;
   const StringRef OverrideSpelling;
   const StringRef FinalSpelling;
 };
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 f8f34794af749..d3ed73ef35a51 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
@@ -4,7 +4,8 @@ modernize-use-override
 ======================
 
 Adds ``override`` (introduced in C++11) to overridden virtual functions and
-removes ``virtual`` from those functions as it is not required.
+removes ``virtual`` from those functions (unless the `AllowVirtual` option is
+used).
 
 ``virtual`` on non base class implementations was used to help indicate to the
 user that a function was virtual. C++ compilers did not use the presence of
@@ -40,6 +41,11 @@ Options
    members, such as ``gcc -Wsuggest-override``/``gcc -Werror=suggest-override``.
    Default is `false`.
 
+.. option:: AllowVirtual
+
+   If set to `true`, ``virtual`` will not be removed by this check when adding
+   ``override`` or ``final``. Default is `false`.
+
 .. 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-virtual.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-allow-virtual.cpp
new file mode 100644
index 0000000000000..17be170e0d9cd
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/use-override-allow-virtual.cpp
@@ -0,0 +1,25 @@
+// RUN: %check_clang_tidy %s modernize-use-override %t -- \
+// RUN:   -config="{CheckOptions: {modernize-use-override.AllowVirtual: true}}"
+
+struct Base {
+  virtual ~Base();
+  virtual void a();
+  virtual void b();
+  virtual void c();
+};
+
+struct Derived : public Base {
+  virtual ~Derived() override;
+
+  virtual void a() override;
+  // CHECK-MESSAGES-NOT: warning:
+  // CHECK-FIXES: {{^}}  virtual void a() override;
+
+  virtual void b();
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: add 'override'
+  // CHECK-FIXES: {{^}}  virtual void b() override;
+
+  void c();
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: annotate
+  // CHECK-FIXES: {{^}}  void c() override;
+};

``````````

</details>


https://github.com/llvm/llvm-project/pull/144916