Enable unnecessary-virtual-specifier by default #133265
Conversation
llvmbot
added
cmake
|
@zmodem Do you mind taking a look? |
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Devon Loehr (DKLoehr) ChangesThis turns on the unnecessary-virtual-specifier warning in genera, 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 The LLVM cleanup was more surprising: I discovered that we have an old policy about including out-of-line virtual functions in every class with a vtable, even 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. Full diff: https://github.com/llvm/llvm-project/pull/133265.diff 3 Files Affected:
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">;
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)
|
There was a problem hiding this comment.
Looks like you’ll have to update some tests because CI is failing (I’d be very surprised if we didn’t have tests for this). Also, this still needs a release note.
Maybe a fix for the virtual anchor pattern would be to somehow not emit the warning for the first (out-of-line) virtual function in a class, but that feels like too much of a hack...
As to whether this should be enabled by default, I don’t really have strong opinions on that. It does seem like you’d usually not want to introduce new virtual functions in a final class; the anchor is really the only use case I can think of.
|
Ach, I was so focused on making the build work that I forgot to run tests... |
|
Ah, missed that, sorry. It seems there hasn't been a release since the warning was added, and the existing release note seems to still apply: llvm-project/clang/docs/ReleaseNotes.rst Line 291 in 8bdcd0a |
Ah, I thought it was an older warning. In that case we don’t need a release note |
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.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/7809 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/8938 Here is the relevant piece of the build log for the reference |
|
Wait, on by default? Perhaps I'm out of line with current thinking here, but IMO, on-by-default should only diagnose things which are likely to be harmful -- NOT just a style or inefficiency issue, which seems to be all this is diagnosing. That LLVM's style conflicts with it also seems like a good indication that it should not be an on by default warning. I'd actually say it belongs in |
I think this one is on the fence. Considering code like: This isn't harmful in the sense of finding a correctness issue because, presumably, something elsewhere tries to derive from that class and gets an error. And in the absence of a derived class, there's no semantic issues with the code. However, it is still mildly harmful if the person writing this code misses the I can't think of a situation where someone would write |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/42/builds/3933 Here is the relevant piece of the build log for the reference |
|
LLVM's intentional usage is either to reduce redundant vtable emission via ensuring there is a key method, OR, to intentionally create a useless vtable, in order to reduce redundant debuginfo emission (it can be emitted only in the vtable's TU when there's a vtable.) I expect both are likely to be rather unusual. I'd say it's highly likely that most code where this diagnostic triggers is simply because "virtual" was added to a function which simply doesn't need it, and the solution is to remove "virtual". This diagnostic can definitely be useful -- it likely indicates an issue in the code. I just don't think the issue it identifies is that important as to warrant being on by default, such that we'd emit this even on a project with has not enabled any warning flags. E.g. as comparison, is this really more important than |
|
To give my point of view, I judge "on-by-default worthiness" roughly by a combination of the false-positive rate and the difficulty of silencing the warning when false positives occur. I'm defining false positives as "a warning instance which the code author thinks isn't worth fixing". In other words, how much effort are we imposing on people by forcing them to deal with this warning? In this case, what I've seen is that the warning catches real issues in most codebases, and it does so without any false positives that I noticed. Fixing instances of the warning is trivial (just remove "virtual"), and can result in minor but real code improvements. The only exception I've seen is LLVM, where pretty much all instances of the warning were false positives. However, since that's due to a project-wide policy, the right solution is to simply disable the warning as not-applicable. The problematic case IMO is where false positives are mixed with real positives, so you'd like to enable the warning but can't do it without convoluting your code to suppress FPs. I haven't seen that here. tl;dr: Yes, this warning provides only a small benefit, but the cost (to users) is also small, so I think it makes sense for it to be enabled by default. |
This reverts commit 4007de0.
Reverts #133265 This causes the whole libc++ CI to fail, since we're not building against a compiler built from current trunk. Specifically, the CMake changes causes some feature detection to fail, resulting in CMake being unable to configure libc++.
…#134105) 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. Specifically, the CMake changes causes some feature detection to fail, resulting in CMake being unable to configure libc++.
|
I view on-by-default warnings as being something we should only add when the code is possibly dangerous. That is, if you don't at least enable |
|
@DKLoehr @AaronBallman Did you see the revert? |
Thank you for the ping, I did not see the revert. Thanks for catching that and sorry for the disruption! Given that this disrupts both LLVM and libc++, I think that's plenty of signal that "on by default" isn't really the best approach here. I think moving it to |
Sounds good to me, and I think James makes a good argument. Does that actually help LLVM and libc++ though? I think at least LLVM does enable -Wextra. |
Effectively a reland of #133265, though due to discussion there we add the warning to -Wextra instead of turning it on by default. We still need to disable it for LLVM due to our unusual policy of using virtual `anchor` functions even in final classes. We now check if the warning exists before disabling it in LLVM builds, so hopefully this will fix the issues libcxx ran into last time. From the previous PR: I've been working on cleaning up this warning in two codebases: LLVM and chromium (plus its dependencies). The chromium + dependency 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.
This turns on the unnecessary-virtual-specifier warning in genera, 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
finalwas 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 about including out-of-line virtual functions in every class with a vtable, even
finalones. 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.