[all-commits] [llvm/llvm-project] 881059: Add unnecessary-virtual-specifier to -Wextra (#138...

[Devon Loehr via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 8810595068a3f17c444e7f96733a6cd9dc08987e
      https://github.com/llvm/llvm-project/commit/8810595068a3f17c444e7f96733a6cd9dc08987e
  Author: Devon Loehr <DKLoehr at users.noreply.github.com>
  Date:   2025-05-07 (Wed, 07 May 2025)

  Changed paths:
    M clang/docs/ReleaseNotes.rst
    M clang/include/clang/Basic/DiagnosticGroups.td
    M llvm/cmake/modules/HandleLLVMOptions.cmake

  Log Message:
  -----------
  Add unnecessary-virtual-specifier to -Wextra (#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.



To unsubscribe from these emails, change your notification settings at https://github.com/llvm/llvm-project/settings/notifications