Revert "Enable unnecessary-virtual-specifier by default"

[philnik777]

Reverts #133265

This causes the whole libc++ CI to fail, since we're not building against a compiler built from current trunk.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Nikolas Klauser (philnik777)

Changes

Reverts llvm/llvm-project#133265

This causes the whole libc++ CI to fail, since we're not building against a compiler built from current trunk.

---

Full diff: https://github.com/llvm/llvm-project/pull/134105.diff

7 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticGroups.td (+4-3)
• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+1-1)
• (modified) clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp (+1-1)
• (modified) clang/test/CXX/class/p2-0x.cpp (+1-1)
• (modified) clang/test/SemaCXX/MicrosoftExtensions.cpp (-1)
• (modified) clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp (+2-3)
• (modified) llvm/cmake/modules/HandleLLVMOptions.cmake (-6)

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 16e1cd4dade8b..09f423ad4b4bd 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -377,12 +377,13 @@ 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) 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.
+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``.
   }];
 }
 
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 2306cb4d7cac4..10568a5ee87fc 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>;
+  InGroup<WarnUnnecessaryVirtualSpecifier>, DefaultIgnore;
 
 // 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 106091b240af6..4209db14eaa52 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 -Wno-unnecessary-virtual-specifier
+// RUN: %clang_analyze_cc1 -analyzer-checker=webkit.RefCntblBaseVirtualDtor -verify %s
 
 #include "mock-types.h"
 
diff --git a/clang/test/CXX/class/p2-0x.cpp b/clang/test/CXX/class/p2-0x.cpp
index 2043486457baf..5b39e0ada7e2c 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'}} expected-warning {{virtual method 'func' is inside a 'final' class}}}
+struct A final { virtual void func() = 0; }; // expected-warning {{abstract class is marked 'final'}} expected-note {{unimplemented pure virtual method 'func' in 'A'}}
 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 9f6939c1681c9..7454a01158f6b 100644
--- a/clang/test/SemaCXX/MicrosoftExtensions.cpp
+++ b/clang/test/SemaCXX/MicrosoftExtensions.cpp
@@ -470,7 +470,6 @@ 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 c9c8c11e1d7ff..a96aa4436e818 100644
--- a/clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
+++ b/clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
@@ -1,6 +1,5 @@
-// 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
+// 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
 
 class A {
     ~A();
diff --git a/llvm/cmake/modules/HandleLLVMOptions.cmake b/llvm/cmake/modules/HandleLLVMOptions.cmake
index f50f60ec0023f..185c9b63aada3 100644
--- a/llvm/cmake/modules/HandleLLVMOptions.cmake
+++ b/llvm/cmake/modules/HandleLLVMOptions.cmake
@@ -690,12 +690,6 @@ 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)

[philnik777]

I think we need some better transition story here. Maybe don't enable the warning by default for time until we've had time to update the libc++ containers?

[philnik777]

CC @DKLoehr @AaronBallman @zmodem - I'm landing this now, since this halts basically all libc++ development.

[DKLoehr]

Thanks for the revert, can you link to a failed build or something so I know what exactly went wrong? Was the issue the presence of an unrecognized warning flag in the build files?

[zmodem]

This causes the whole libc++ CI to fail, since we're not building against a compiler built from current trunk.

Can you provide an example, such as a link to a failing build, or an explanation of the failure? I found https://buildkite.com/llvm-project/libcxx-ci/builds?created_from=2025-04-01&created_to=2025-04-01 but didn't see any failures related to Devon's change there.