Warn about virtual methods in final classes
#131188
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Devon Loehr (DKLoehr) ChangesThere's never any point to adding a We don't currently implement the second part of the suggestion, to warn on I tested the warning by compiling chromium and clang itself. Chromium resulted in 277 warnings across 109 files, while clang had 38 warnings across 29 files. I inspected a subset of the warning sites manually, and they all seemed legitimate. This warning is very easy to fix (just remove the Full diff: https://github.com/llvm/llvm-project/pull/131188.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 8989124611e66..15858631c17b7 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -247,6 +247,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..e785e84205192 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -372,6 +372,17 @@ 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.
+ }];
+}
+
// 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 8e6e6e892cdd7..ed31c2d225073 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 dd779ee377309..1b2e494956d4b 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7193,10 +7193,15 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
// class without overriding any.
if (!M->isStatic())
DiagnoseHiddenVirtualMethods(M);
+
if (M->hasAttr<OverrideAttr>())
HasMethodWithOverrideControl = true;
else if (M->size_overridden_methods() > 0)
HasOverridingMethodWithoutOverrideControl = true;
+
+ // See if a method is marked as virtual inside of a final class.
+ 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..00f696bfa0d4f
--- /dev/null
+++ b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
@@ -0,0 +1,27 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier %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 virt(int);
+ int nonvirt();
+};
+
+struct Bar final : BarBase {
+ ~Bar() override = delete;
+ void virt() override {};
+ virtual int virt(int) override; // expected-warning {{virtual method}}
+ int nonvirt();
+};
|
|
@zmodem Would you take a look? What do you think about implementing the suggestion about checking if virtual methods actually get overridden or not? |
Since we could only do it for classes with internal linkage, I think it's probably not very valuable. Warning about virtual functions that are not overriding anything and cannot be overridden because |
|
Changed to not warn on |
| @@ -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; | |||
There was a problem hiding this comment.
Instead of DefaultIgnore, could we turn this on by default? I think it should be pretty high fidelity. Or at least we could put it in -Wall or -Wextra?
There was a problem hiding this comment.
My intention is to go through and try to clean up all the instances where it triggers in a followup PR, so we wouldn't have this one clogged with lots of tiny changes in random files. I agree it should be on by default.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/134/builds/15203 Here is the relevant piece of the build log for the reference |
- `Threading::Thread` is not polymorphic, there is no need for a virtual destructor. - `HTMLAnchorElement::has_download_preference` isn't overridden by anything. This warning was introduced in llvm/llvm-project#131188.
`ConservativeVector`, `RootVector` and `RootHashMap` are final types, and their base classes have a protected destructor, so when their destructor is called, the static and dynamic types will be the same (can't destruct them through a pointer to a base or derived class). Therefore, there is no need for a virtual destructor. This fixes the newly introduced `-Wunnecessary-virtual-specifier` Clang warning (llvm/llvm-project#131188).
- `Threading::Thread` is not polymorphic, there is no need for a virtual destructor. - `HTMLAnchorElement::has_download_preference` isn't overridden by anything. This warning was introduced in llvm/llvm-project#131188.
There's never any point to adding a
virtualspecifier to methods in afinalclass, 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
virtualmethods 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, while clang had 38 warnings across 29 files. I inspected a subset of the warning sites manually, and they all seemed legitimate.
This warning is very easy to fix (just remove the
virtualspecifier) 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.