https://github.com/DKLoehr updated
https://github.com/llvm/llvm-project/pull/131188
>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/4] 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/4] 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/4] 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;
>From eb03ecf974d475021233cdf63e50dad804132bff Mon Sep 17 00:00:00 2001
From: Devon Loehr <dlo...@google.com>
Date: Fri, 14 Mar 2025 14:24:13 +0000
Subject: [PATCH 4/4] Don't warn on virtual...override.
---
clang/include/clang/Basic/DiagnosticGroups.td | 2 ++
clang/lib/Sema/SemaDeclCXX.cpp | 18 +++++++++++-------
.../SemaCXX/unnecessary-virtual-specifier.cpp | 17 +++++++++--------
3 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td
b/clang/include/clang/Basic/DiagnosticGroups.td
index e785e84205192..e54f921741269 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -380,6 +380,8 @@ 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.
+
+The warning does not fire on virtual methods which are also marked
``override``.
}];
}
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index 1b2e494956d4b..381b81b5b832a 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7194,14 +7194,18 @@ void Sema::CheckCompletedCXXClass(Scope *S,
CXXRecordDecl *Record) {
if (!M->isStatic())
DiagnoseHiddenVirtualMethods(M);
- if (M->hasAttr<OverrideAttr>())
+ 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;
+ } else {
+ if (M->size_overridden_methods() > 0)
+ HasOverridingMethodWithoutOverrideControl = true;
+
+ // Warn on virtual methods in final classes, unless they're also
+ // marked `override`.
+ 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
index 00f696bfa0d4f..b1fa6145e34e1 100644
--- a/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
+++ b/clang/test/SemaCXX/unnecessary-virtual-specifier.cpp
@@ -2,10 +2,10 @@
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 ~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}}
@@ -14,14 +14,15 @@ struct Foo final {
struct BarBase {
virtual ~BarBase() = delete;
- virtual void virt() {}
- virtual int virt(int);
+ 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}}
+ void virt() override {};
+ // `virtual ... override;` is a common pattern, so don't warn
+ virtual int virt(int) override;
int nonvirt();
};
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
