[clang] [llvm] Enable unnecessary-virtual-specifier by default (PR #133265)
Devon Loehr via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 28 07:44:35 PDT 2025
https://github.com/DKLoehr updated https://github.com/llvm/llvm-project/pull/133265
>From 792e1f3d062415134b8dfc4e8ed52f769f3e01f8 Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Thu, 27 Mar 2025 14:59:44 +0000
Subject: [PATCH 1/4] Enable by default
---
clang/include/clang/Basic/DiagnosticGroups.td | 7 +++----
clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 +-
2 files changed, 4 insertions(+), 5 deletions(-)
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 c77cde297dc32..05ded7d9ecef1 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">;
>From 5a221ac1f11d3dfde2a241be65bd06af25c0c37f Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Thu, 27 Mar 2025 15:18:30 +0000
Subject: [PATCH 2/4] Disable for LLVM
---
llvm/cmake/modules/HandleLLVMOptions.cmake | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake b/llvm/cmake/modules/HandleLLVMOptions.cmake
index 185c9b63aada3..f2c5cf4241e3d 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -689,7 +689,7 @@ if ( LLVM_COMPILER_IS_GCC_COMPATIBLE OR CMAKE_CXX_COMPILER_ID MATCHES "XL" )
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)
+ append("-Werror=unguarded-availability-new -Wno-unnecessary-virtual-specifier" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
endif()
if (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND LLVM_ENABLE_LTO)
>From fff138d2047e2867a969d36a999eba5f9722761f Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Thu, 27 Mar 2025 18:45:37 +0000
Subject: [PATCH 3/4] Disable in tests
---
.../Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp | 2 +-
clang/test/CXX/class/p2-0x.cpp | 2 +-
clang/test/SemaCXX/MicrosoftExtensions.cpp | 1 +
clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp | 5 +++--
4 files changed, 6 insertions(+), 4 deletions(-)
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();
>From db0fd6c8f5f594595c223b0b17280293b82023ee Mon Sep 17 00:00:00 2001
From: Devon Loehr <dloehr at google.com>
Date: Fri, 28 Mar 2025 14:44:18 +0000
Subject: [PATCH 4/4] Add version check and explanation
---
llvm/cmake/modules/HandleLLVMOptions.cmake | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake b/llvm/cmake/modules/HandleLLVMOptions.cmake
index f2c5cf4241e3d..f50f60ec0023f 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -689,7 +689,13 @@ if ( LLVM_COMPILER_IS_GCC_COMPATIBLE OR CMAKE_CXX_COMPILER_ID MATCHES "XL" )
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 -Wno-unnecessary-virtual-specifier" CMAKE_C_FLAGS CMAKE_CXX_FLAGS)
+ 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