[llvm] 4007de0 - Enable unnecessary-virtual-specifier by default (#133265)
via llvm-commits
llvm-commits at lists.llvm.org
Mon Mar 31 07:28:58 PDT 2025
Author: Devon Loehr
Date: 2025-03-31T16:28:53+02:00
New Revision: 4007de00a0574141695ace7a8d34aaf740a2c2e4
URL: https://github.com/llvm/llvm-project/commit/4007de00a0574141695ace7a8d34aaf740a2c2e4
DIFF: https://github.com/llvm/llvm-project/commit/4007de00a0574141695ace7a8d34aaf740a2c2e4.diff
LOG: Enable unnecessary-virtual-specifier by default (#133265)
This turns on the unnecessary-virtual-specifier warning in general, but
disables it when building LLVM. It also tweaks the warning description
to be slightly more accurate.
Background: I've been working on cleaning up this warning in two
codebases: LLVM and chromium (plus its dependencies). The chromium
cleanup has been straightforward. Git archaeology shows that there are
two reasons for the warnings: classes to which `final` was added after
they were initially committed, and classes with virtual destructors that
nobody remarks on. Presumably the latter case is because people are just
very used to destructors being virtual.
The LLVM cleanup was more surprising: I discovered that we have an [old
policy](https://llvm.org/docs/CodingStandards.html#provide-a-virtual-method-anchor-for-classes-in-headers)
about including out-of-line virtual functions in every class with a
vtable, even `final` ones. This means our codebase has many virtual
"anchor" functions which do nothing except control where the vtable is
emitted, and which trigger the warning. I looked into alternatives to
satisfy the policy, such as using destructors instead of introducing a
new function, but it wasn't clear if they had larger implications.
Overall, it seems like the warning is genuinely useful in most codebases
(evidenced by chromium and its dependencies), and LLVM is an unusual
case. Therefore we should enable the warning by default, and turn it off
only for LLVM builds.
Added:
Modified:
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
clang/test/CXX/class/p2-0x.cpp
clang/test/SemaCXX/MicrosoftExtensions.cpp
clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
llvm/cmake/modules/HandleLLVMOptions.cmake
Removed:
################################################################################
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index b9f08d96151c9..e6e9ebbc2c304 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -377,13 +377,12 @@ 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.
+destructors) that does not override anything. Since ``final`` classes cannot be
+subclassed, their methods cannot be overridden, so there is no point to
+introducing new ``virtual`` methods.
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``.
}];
}
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index b03926db8170a..5e45482584946 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2733,7 +2733,7 @@ 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;
+ InGroup<WarnUnnecessaryVirtualSpecifier>;
// C++11 attributes
def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
diff --git a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
index 4209db14eaa52..106091b240af6 100644
--- a/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
+++ b/clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s -Wno-unnecessary-virtual-specifier
#include "mock-types.h"
diff --git a/clang/test/CXX/class/p2-0x.cpp b/clang/test/CXX/class/p2-0x.cpp
index 5b39e0ada7e2c..2043486457baf 100644
--- a/clang/test/CXX/class/p2-0x.cpp
+++ b/clang/test/CXX/class/p2-0x.cpp
@@ -28,7 +28,7 @@ struct C : A<int> { }; // expected-error {{base 'A' is marked 'final'}}
namespace Test4 {
-struct A final { virtual void func() = 0; }; // expected-warning {{abstract class is marked 'final'}} expected-note {{unimplemented pure virtual method 'func' in 'A'}}
+struct A final { virtual void func() = 0; }; // expected-warning {{abstract class is marked 'final'}} expected-note {{unimplemented pure virtual method 'func' in 'A'}} expected-warning {{virtual method 'func' is inside a 'final' class}}}
struct B { virtual void func() = 0; }; // expected-note {{unimplemented pure virtual method 'func' in 'C'}}
struct C final : B { }; // expected-warning {{abstract class is marked 'final'}}
diff --git a/clang/test/SemaCXX/MicrosoftExtensions.cpp b/clang/test/SemaCXX/MicrosoftExtensions.cpp
index 7454a01158f6b..9f6939c1681c9 100644
--- a/clang/test/SemaCXX/MicrosoftExtensions.cpp
+++ b/clang/test/SemaCXX/MicrosoftExtensions.cpp
@@ -470,6 +470,7 @@ struct InheritFromSealed : SealedType {};
class SealedDestructor { // expected-note {{mark 'SealedDestructor' as 'sealed' to silence this warning}}
// expected-warning at +1 {{'sealed' keyword is a Microsoft extension}}
virtual ~SealedDestructor() sealed; // expected-warning {{class with destructor marked 'sealed' cannot be inherited from}}
+ // expected-warning at -1 {{virtual method '~SealedDestructor' is inside a 'final' class}}
};
// expected-warning at +1 {{'abstract' keyword is a Microsoft extension}}
diff --git a/clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp b/clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
index a96aa4436e818..c9c8c11e1d7ff 100644
--- a/clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
+++ b/clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
@@ -1,5 +1,6 @@
-// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s -Wfinal-dtor-non-final-class
-// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -Wfinal-dtor-non-final-class -fdiagnostics-parseable-fixits 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s -Wfinal-dtor-non-final-class -Wno-unnecessary-virtual-specifier
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -Wfinal-dtor-non-final-class -Wno-unnecessary-virtual-specifier \
+// RUN: -fdiagnostics-parseable-fixits 2>&1 | FileCheck %s
class A {
~A();
diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake b/llvm/cmake/modules/HandleLLVMOptions.cmake
index 185c9b63aada3..f50f60ec0023f 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -690,6 +690,12 @@ endif( LLVM_COMPILER_IS_GCC_COMPATIBLE OR CMAKE_CXX_COMPILER_ID MATCHES "XL" )
if (CMAKE_CXX_COMPILER_ID MATCHES "Clang")
append("-Werror=unguarded-availability-new" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+ if (CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 21.0)
+ # LLVM has a policy of including virtual "anchor" functions to control
+ # where the vtable is emitted. In `final` classes, these are exactly what
+ # this warning detects: unnecessary virtual methods.
+ append("-Wno-unnecessary-virtual-specifier" CMAKE_CXX_FLAGS)
+ endif()
endif()
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND LLVM_ENABLE_LTO)
More information about the llvm-commits
mailing list