[all-commits] [llvm/llvm-project] 4007de: Enable unnecessary-virtual-specifier by default (#...

[Devon Loehr via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 4007de00a0574141695ace7a8d34aaf740a2c2e4
      https://github.com/llvm/llvm-project/commit/4007de00a0574141695ace7a8d34aaf740a2c2e4
  Author: Devon Loehr <DKLoehr at users.noreply.github.com>
  Date:   2025-03-31 (Mon, 31 Mar 2025)

  Changed paths:
    M clang/include/clang/Basic/DiagnosticGroups.td
    M clang/include/clang/Basic/DiagnosticSemaKinds.td
    M clang/test/Analysis/Checkers/WebKit/ref-cntbl-crtp-base-no-virtual-dtor.cpp
    M clang/test/CXX/class/p2-0x.cpp
    M clang/test/SemaCXX/MicrosoftExtensions.cpp
    M clang/test/SemaCXX/warn-final-dtor-non-final-class.cpp
    M llvm/cmake/modules/HandleLLVMOptions.cmake

  Log Message:
  -----------
  Enable unnecessary-virtual-specifier by default (#133265)

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.



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