Author: Devon Loehr
Date: 2025-03-17T15:07:31+01:00
New Revision: 2ff370f45266b14c2a86e6395042a4574701f2d2

URL: 
https://github.com/llvm/llvm-project/commit/2ff370f45266b14c2a86e6395042a4574701f2d2
DIFF: 
https://github.com/llvm/llvm-project/commit/2ff370f45266b14c2a86e6395042a4574701f2d2.diff

LOG: Warn about virtual methods in `final` classes (#131188)

There's never any point to adding a `virtual` specifier to methods in a
`final` class, since the class can't be subclassed. This adds a warning
when we notice this happening, as suggested in #131108.

We don't currently implement the second part of the suggestion, to warn
on `virtual` methods which are never overridden anywhere. Although it's
feasible to do this for things with internal linkage (so we can check at
the end of the TU), it's more complicated to implement and it's not
clear it's worth the effort.

I tested the warning by compiling chromium and clang itself. Chromium
resulted in [277 warnings across 109
files](https://github.com/user-attachments/files/19234889/warnings-chromium.txt),
while clang had [38 warnings across 29
files](https://github.com/user-attachments/files/19234888/warnings-clang.txt).
I inspected a subset of the warning sites manually, and they all seemed
legitimate.

This warning is very easy to fix (just remove the `virtual` specifier)
and I haven't seen any false positives, so it's suitable for
on-by-default. However, I've currently made it off-by-default because it
fires at several places in the repo. I plan to submit a followup PR
fixing those places and enabling the warning by default.

Added: 
    clang/test/SemaCXX/unnecessary-virtual-specifier.cpp

Modified: 
    clang/docs/ReleaseNotes.rst
    clang/include/clang/Basic/DiagnosticGroups.td
    clang/include/clang/Basic/DiagnosticSemaKinds.td
    clang/lib/Sema/SemaDeclCXX.cpp

Removed: 
    


################################################################################
diff  --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2a1c5ee2d788e..17128f15d345a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -251,6 +251,9 @@ Improvements to Clang's diagnostics
 - The ``-Wsign-compare`` warning now treats expressions with bitwise not(~) 
and minus(-) as signed integers 
   except for the case where the operand is an unsigned integer
   and throws warning if they are compared with unsigned integers (##18878).
+- The ``-Wunnecessary-virtual-specifier`` warning has been added to warn about
+  methods which are marked as virtual inside a ``final`` class, and hence can
+  never be overridden.
 
 - Improve the diagnostics for chained comparisons to report actual expressions 
and operators (#GH129069).
 

diff  --git a/clang/include/clang/Basic/DiagnosticGroups.td 
b/clang/include/clang/Basic/DiagnosticGroups.td
index fac80fb4009aa..e54f921741269 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -372,6 +372,19 @@ def CXX11WarnInconsistentOverrideMethod :
 def CXX11WarnSuggestOverrideDestructor : 
DiagGroup<"suggest-destructor-override">;
 def CXX11WarnSuggestOverride : DiagGroup<"suggest-override">;
 
+def WarnUnnecessaryVirtualSpecifier : 
DiagGroup<"unnecessary-virtual-specifier"> {
+  code Documentation = [{
+Warns when a ``final`` class contains a virtual method (including virtual
+destructors). Since ``final`` classes cannot be subclassed, their methods
+cannot be overridden, and hence the ``virtual`` specifier is useless.
+
+The warning also detects virtual methods in classes whose destructor is
+``final``, for the same reason.
+
+The warning does not fire on virtual methods which are also marked 
``override``.
+  }];
+}
+
 // Original name of this warning in Clang
 def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>;
 

diff  --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td 
b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 87008cd9a8f65..627bebb31fc8d 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2706,6 +2706,9 @@ def warn_final_dtor_non_final_class : Warning<
   InGroup<FinalDtorNonFinalClass>;
 def note_final_dtor_non_final_class_silence : Note<
   "mark %0 as '%select{final|sealed}1' to silence this warning">;
+def warn_unnecessary_virtual_specifier : Warning<
+  "virtual method %0 is inside a 'final' class and can never be overridden">,
+  InGroup<WarnUnnecessaryVirtualSpecifier>, DefaultIgnore;
 
 // C++11 attributes
 def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;

diff  --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index a02bd8335fa20..8850161ab01c7 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7193,10 +7193,18 @@ void Sema::CheckCompletedCXXClass(Scope *S, 
CXXRecordDecl *Record) {
         // class without overriding any.
         if (!M->isStatic())
           DiagnoseHiddenVirtualMethods(M);
-        if (M->hasAttr<OverrideAttr>())
+
+        if (M->hasAttr<OverrideAttr>()) {
           HasMethodWithOverrideControl = true;
-        else if (M->size_overridden_methods() > 0)
+        } else if (M->size_overridden_methods() > 0) {
           HasOverridingMethodWithoutOverrideControl = true;
+        } else {
+          // Warn on newly-declared virtual methods in `final` classes
+          if (M->isVirtualAsWritten() && Record->isEffectivelyFinal()) {
+            Diag(M->getLocation(), diag::warn_unnecessary_virtual_specifier)
+                << M;
+          }
+        }
       }
 
       if (!isa<CXXDestructorDecl>(M))

diff  --git a/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp 
b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
new file mode 100644
index 0000000000000..789710eccf729
--- /dev/null
+++ b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier 
-Wno-inconsistent-missing-override %s
+
+struct Foo final {
+  Foo() = default;
+  virtual ~Foo() = default;                      // expected-warning {{virtual 
method}}
+  virtual Foo& operator=(Foo& other) = default;  // expected-warning {{virtual 
method}}
+  virtual Foo& operator=(Foo&& other) = default; // expected-warning {{virtual 
method}}
+  void f();
+  virtual void f(int);                           // expected-warning {{virtual 
method}}
+  int g(int x) { return x; };
+  virtual int g(bool);                           // expected-warning {{virtual 
method}}
+  static int s();
+};
+
+struct BarBase {
+  virtual ~BarBase() = delete;
+  virtual void virt() {}
+  virtual int  virt2(int);
+  virtual bool virt3(bool);
+  int nonvirt();
+};
+
+struct Bar final : BarBase {
+  ~Bar() override = delete;
+          void virt() override {};
+  virtual int  virt2(int) override;               // `virtual ... override;` 
is a common pattern, so don't warn
+  virtual bool virt3(bool);                       // Already virtual in the 
base class; triggers
+                                                  // 
-Winconsistent-missing-override or -Wsuggest-override instead
+  virtual int  new_virt(bool);                    // expected-warning 
{{virtual method}}
+  int nonvirt();
+};


        
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to