https://github.com/DKLoehr created https://github.com/llvm/llvm-project/pull/138741
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. >From 964841d9a033aafdbb2e27d77644f6ce9f4883a0 Mon Sep 17 00:00:00 2001 From: Devon Loehr <dlo...@google.com> Date: Tue, 6 May 2025 17:28:48 +0000 Subject: [PATCH] Add unnecessary-virtual-specifier to -Wextra --- clang/include/clang/Basic/DiagnosticGroups.td | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 1faf8508121f4..bb6e7070b1574 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -421,13 +421,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``. }]; } @@ -1163,6 +1162,7 @@ def Extra : DiagGroup<"extra", [ FUseLdPath, CastFunctionTypeMismatch, InitStringTooLongMissingNonString, + WarnUnnecessaryVirtualSpecifier, ]>; def Most : DiagGroup<"most", [ _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
