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@+1 {{'sealed' keyword is a Microsoft extension}} virtual ~SealedDestructor() sealed; // expected-warning {{class with destructor marked 'sealed' cannot be inherited from}} + // expected-warning@-1 {{virtual method '~SealedDestructor' is inside a 'final' class}} }; // expected-warning@+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) _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
