https://github.com/DKLoehr created https://github.com/llvm/llvm-project/pull/131188
There's never any point to adding a `virtual` specifier to methods in a `final` class, since the class can't be subclassed. This adds a warning when we notice this happening, as suggested in #131108. We don't currently implement the second part of the suggestion, to warn on `virtual` methods which are never overridden anywhere. Although it's feasible to do this for things with internal linkage (so we can check at the end of the TU), it's more complicated to implement and it's not clear it's worth the effort. I tested the warning by compiling chromium and clang itself. Chromium resulted in [277 warnings across 109 files](https://github.com/user-attachments/files/19234889/warnings-chromium.txt), while clang had [38 warnings across 29 files](https://github.com/user-attachments/files/19234888/warnings-clang.txt). I inspected a subset of the warning sites manually, and they all seemed legitimate. This warning is very easy to fix (just remove the `virtual` specifier), so it's suitable for on-by-default. However, I've currently made it off-by-default because it fires at several places in the repo. I plan to submit a followup PR fixing those places and enabling the warning by default. >From fbd474fb5ae3adeaf1644a4d44e916e4d7c66395 Mon Sep 17 00:00:00 2001 From: Devon Loehr <dlo...@google.com> Date: Thu, 13 Mar 2025 17:34:27 +0000 Subject: [PATCH 1/3] Initial warning commit --- clang/include/clang/Basic/DiagnosticGroups.td | 2 ++ .../clang/Basic/DiagnosticSemaKinds.td | 3 +++ clang/lib/Sema/SemaDeclCXX.cpp | 5 ++++ .../SemaCXX/unnecessary-virtual-specifier.cpp | 27 +++++++++++++++++++ 4 files changed, 37 insertions(+) create mode 100644 clang/test/SemaCXX/unnecessary-virtual-specifier.cpp diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index fac80fb4009aa..65593ddcb8608 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -372,6 +372,8 @@ def CXX11WarnInconsistentOverrideMethod : def CXX11WarnSuggestOverrideDestructor : DiagGroup<"suggest-destructor-override">; def CXX11WarnSuggestOverride : DiagGroup<"suggest-override">; +def WarnUnnecessaryVirtualSpecifier : DiagGroup<"unnecessary-virtual-specifier">; + // Original name of this warning in Clang def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 8e6e6e892cdd7..a87cf7e674b4c 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -2706,6 +2706,9 @@ def warn_final_dtor_non_final_class : Warning< InGroup<FinalDtorNonFinalClass>; 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>; // C++11 attributes def err_repeat_attribute : Error<"%0 attribute cannot be repeated">; diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp index dd779ee377309..1b2e494956d4b 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -7193,10 +7193,15 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) { // class without overriding any. if (!M->isStatic()) DiagnoseHiddenVirtualMethods(M); + if (M->hasAttr<OverrideAttr>()) HasMethodWithOverrideControl = true; else if (M->size_overridden_methods() > 0) HasOverridingMethodWithoutOverrideControl = true; + + // See if a method is marked as virtual inside of a final class. + if (M->isVirtualAsWritten() && Record->isEffectivelyFinal()) + Diag(M->getLocation(), diag::warn_unnecessary_virtual_specifier) << M; } if (!isa<CXXDestructorDecl>(M)) diff --git a/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp new file mode 100644 index 0000000000000..eb8397d9ade45 --- /dev/null +++ b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp @@ -0,0 +1,27 @@ +// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier -Wsuggest-override %s + +struct Foo final { + Foo() = default; + virtual ~Foo() = default; // expected-warning {{virtual method}} + virtual Foo& operator=(Foo& other) = default; // expected-warning {{virtual method}} + virtual Foo& operator=(Foo&& other) = default; // expected-warning {{virtual method}} + void f(); + virtual void f(int); // expected-warning {{virtual method}} + int g(int x) { return x; }; + virtual int g(bool); // expected-warning {{virtual method}} + static int s(); +}; + +struct BarBase { + virtual ~BarBase() = delete; + virtual void virt() {} + virtual int virt(int); + int nonvirt(); +}; + +struct Bar final : BarBase { + ~Bar() override = delete; + void virt() override {}; + virtual int virt(int) override; // expected-warning {{virtual method}} + int nonvirt(); +}; >From 4a16ef81a4882785708a4973f78586119235e8f5 Mon Sep 17 00:00:00 2001 From: Devon Loehr <dlo...@google.com> Date: Thu, 13 Mar 2025 17:59:39 +0000 Subject: [PATCH 2/3] Add documentation --- clang/docs/ReleaseNotes.rst | 3 +++ clang/include/clang/Basic/DiagnosticGroups.td | 11 ++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 8989124611e66..15858631c17b7 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -247,6 +247,9 @@ Improvements to Clang's diagnostics - The ``-Wsign-compare`` warning now treats expressions with bitwise not(~) and minus(-) as signed integers except for the case where the operand is an unsigned integer and throws warning if they are compared with unsigned integers (##18878). +- The ``-Wunnecessary-virtual-specifier`` warning has been added to warn about + methods which are marked as virtual inside a ``final`` class, and hence can + never be overridden. - Improve the diagnostics for chained comparisons to report actual expressions and operators (#GH129069). diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 65593ddcb8608..e785e84205192 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -372,7 +372,16 @@ def CXX11WarnInconsistentOverrideMethod : def CXX11WarnSuggestOverrideDestructor : DiagGroup<"suggest-destructor-override">; def CXX11WarnSuggestOverride : DiagGroup<"suggest-override">; -def WarnUnnecessaryVirtualSpecifier : DiagGroup<"unnecessary-virtual-specifier">; +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. + +The warning also detects virtual methods in classes whose destructor is +``final``, for the same reason. + }]; +} // Original name of this warning in Clang def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>; >From 11f33b66533f58a5180f58581c21067b9a422b62 Mon Sep 17 00:00:00 2001 From: Devon Loehr <dlo...@google.com> Date: Thu, 13 Mar 2025 18:18:15 +0000 Subject: [PATCH 3/3] Make DefaultIgnore --- clang/include/clang/Basic/DiagnosticSemaKinds.td | 2 +- clang/test/SemaCXX/unnecessary-virtual-specifier.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index a87cf7e674b4c..ed31c2d225073 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -2708,7 +2708,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>; + InGroup<WarnUnnecessaryVirtualSpecifier>, DefaultIgnore; // C++11 attributes def err_repeat_attribute : Error<"%0 attribute cannot be repeated">; diff --git a/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp index eb8397d9ade45..00f696bfa0d4f 100644 --- a/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp +++ b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp @@ -1,4 +1,4 @@ -// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier -Wsuggest-override %s +// RUN: %clang_cc1 -fsyntax-only -verify -Wunnecessary-virtual-specifier %s struct Foo final { Foo() = default; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
