Add unnecessary-virtual-specifier to -Wextra

[DKLoehr]

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 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.

[llvmbot]

@llvm/pr-subscribers-libcxx

@llvm/pr-subscribers-clang

Author: Devon Loehr (DKLoehr)

Changes

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 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.

---

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

1 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticGroups.td (+4-4)

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", [

[DKLoehr]

@zmodem @AaronBallman @Sirraide -- reviewers from previous attempt

@philnik777 -- do you think this PR will work with libc++?

[philnik777]

This should probably work. To make sure you can make a simple change in the libcxx/ subdirectory to trigger the libc++ pre-commit CI (which should of course be removed again before actually committing).

[Sirraide]

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 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.

I wonder if we could add an escape hatch for this somehow—maybe don’t fire the warning on the first virtual function that is declared out-of-line if there’s an easy way of doing that, but that feels like a hack. We could also add an attribute instead to surpress the warning, but is adding an attribute for a single diagnostic really worth it?

Alternatively, we could ‘fix’ our codebase instead by introducing an LLVM_VIRTUAL_ANCHOR macro or sth like that which disables the diagnostic for that one declaration—I guess this probably depends how common this pattern is elsewhere; if there are a lot of people who do this then the attribute might be easier to use.

[zmodem]

lgtm (without the libcxx/src/hash.cpp change obviously)

[DKLoehr]

Looks like all the libc++ checks passed, so we should be good.

Alternatively, we could ‘fix’ our codebase instead by introducing an LLVM_VIRTUAL_ANCHOR macro or sth like that which disables the diagnostic for that one declaration.

This seems like a good way to do it, since it clearly documents what's going on. LLVM is the only project I've seen with such a policy; across all of chromium's dependencies, the extra virtual specifiers we flagged were indeed mistakes. The issue with libc++ was more to do with infrastructure than the warning itself, IIUC.

[Sirraide]

This seems like a good way to do it, since it clearly documents what's going on. LLVM is the only project I've seen with such a policy; across all of chromium's dependencies, the extra virtual specifiers we flagged were indeed mistakes. The issue with libc++ was more to do with infrastructure than the warning itself, IIUC.

Alright, in that case I’d say it would be nice to do that in a follow-up pr so we can stop disabling the warning.

[llvm-ci]

LLVM Buildbot has detected a new failure on builder lld-x86_64-win running on as-worker-93 while building clang,llvm at step 7 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/146/builds/2868

Here is the relevant piece of the build log for the reference

Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'LLVM-Unit :: Support/./SupportTests.exe/90/95' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe-LLVM-Unit-19148-90-95.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=95 GTEST_SHARD_INDEX=90 C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe
--

Script:
--
C:\a\lld-x86_64-win\build\unittests\Support\.\SupportTests.exe --gtest_filter=ProgramEnvTest.CreateProcessLongPath
--
C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(160): error: Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp(163): error: fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied



C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:160
Expected equality of these values:
  0
  RC
    Which is: -2

C:\a\lld-x86_64-win\llvm-project\llvm\unittests\Support\ProgramTest.cpp:163
fs::remove(Twine(LongPath)): did not return errc::success.
error number: 13
error message: permission denied




********************

[llvm-ci]

LLVM Buildbot has detected a new failure on builder clang-ppc64-aix running on aix-ppc64 while building clang,llvm at step 6 "test-build-unified-tree-check-all".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/3453

Here is the relevant piece of the build log for the reference

Step 6 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'lit :: timeout-hang.py' FAILED ********************
Exit Code: 1

Command Output (stdout):
--
# RUN: at line 13
not env -u FILECHECK_OPTS "/home/llvm/llvm-external-buildbots/workers/env/bin/python3.11" /home/llvm/llvm-external-buildbots/workers/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/utils/lit/lit.py -j1 --order=lexical Inputs/timeout-hang/run-nonexistent.txt  --timeout=1 --param external=0 | "/home/llvm/llvm-external-buildbots/workers/env/bin/python3.11" /home/llvm/llvm-external-buildbots/workers/aix-ppc64/clang-ppc64-aix/build/utils/lit/tests/timeout-hang.py 1
# executed command: not env -u FILECHECK_OPTS /home/llvm/llvm-external-buildbots/workers/env/bin/python3.11 /home/llvm/llvm-external-buildbots/workers/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/utils/lit/lit.py -j1 --order=lexical Inputs/timeout-hang/run-nonexistent.txt --timeout=1 --param external=0
# .---command stderr------------
# | lit.py: /home/llvm/llvm-external-buildbots/workers/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/utils/lit/lit/main.py:72: note: The test suite configuration requested an individual test timeout of 0 seconds but a timeout of 1 seconds was requested on the command line. Forcing timeout to be 1 seconds.
# `-----------------------------
# executed command: /home/llvm/llvm-external-buildbots/workers/env/bin/python3.11 /home/llvm/llvm-external-buildbots/workers/aix-ppc64/clang-ppc64-aix/build/utils/lit/tests/timeout-hang.py 1
# .---command stdout------------
# | Testing took as long or longer than timeout
# `-----------------------------
# error: command failed with exit status: 1

--

********************