logan-5 created this revision.
logan-5 added a reviewer: rsmith.
logan-5 added a project: clang.
Herald added a subscriber: cfe-commits.
This patch adds `-Wsuggest-override`, which allows for more aggressive
enforcement of modern C++ best practices, as well as better compatibility with
gcc, which has had its own `-Wsuggest-override` since version 5.1.
Clang already has `-Winconsistent-missing-override`, which only warns in the
case where there is at least one function already marked `override` in a class.
This warning strengthens that warning by suggesting the `override` keyword
regardless of whether it is already present anywhere.
The text between suggest-override and inconsistent-missing-override is now
shared, using `TextSubstitution` for the entire diagnostic text. I'm not sure
if there is a better way to accomplish this.
This patch as written behaves strangely with
`-Wno-inconsistent-missing-override -Wsuggest-override`. In that case, a
warning is //only// generated if there are no overrides already marked (because
inconsistent-missing-override 'wins' when choosing which diagnostic to emit).
However, this is a fairly nonsensical use case, so in my opinion it's fine.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D82728
Files:
clang/include/clang/Basic/DiagnosticGroups.td
clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/include/clang/Sema/Sema.h
clang/lib/Sema/SemaDeclCXX.cpp
clang/test/SemaCXX/warn-suggest-destructor-override
clang/test/SemaCXX/warn-suggest-override
Index: clang/test/SemaCXX/warn-suggest-override
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-override
@@ -0,0 +1,47 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-override
+
+class A {
+ public:
+ ~A() {}
+ void run() {}
+};
+
+class B : public A {
+ public:
+ ~B() {}
+ void run() {}
+};
+
+class C {
+ public:
+ virtual void run() {} // expected-note 2{{overridden virtual function is here}}
+ virtual ~C() {} // expected-note 2{{overridden virtual function is here}}
+};
+
+class D : public C {
+ public:
+ void run() {}
+ // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+ ~D() {}
+ // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+class E : public C {
+ public:
+ virtual void run() {}
+ // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}}
+ virtual ~E() {}
+ // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
+
+class F : public C {
+ public:
+ void run() override {}
+ ~F() override {}
+};
+
+class G : public C {
+ public:
+ void run() final {}
+ ~G() final {}
+};
Index: clang/test/SemaCXX/warn-suggest-destructor-override
===================================================================
--- /dev/null
+++ clang/test/SemaCXX/warn-suggest-destructor-override
@@ -0,0 +1,32 @@
+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override
+
+class A {
+ public:
+ ~A() {}
+ void virtual run() {}
+};
+
+class B : public A {
+ public:
+ ~B() {}
+};
+
+class C {
+ public:
+ virtual void run() {}
+ virtual ~C() {} // expected-note 2{{overridden virtual function is here}}
+};
+
+class D : public C {
+ public:
+ void run() {}
+ ~D() {}
+ // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}}
+};
+
+class E : public C {
+ public:
+ void run() {}
+ virtual ~E() {}
+ // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}}
+};
Index: clang/lib/Sema/SemaDeclCXX.cpp
===================================================================
--- clang/lib/Sema/SemaDeclCXX.cpp
+++ clang/lib/Sema/SemaDeclCXX.cpp
@@ -3045,7 +3045,7 @@
<< MD->getDeclName();
}
-void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent) {
if (D->isInvalidDecl() || D->hasAttr<OverrideAttr>())
return;
CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D);
@@ -3061,12 +3061,19 @@
return;
if (MD->size_overridden_methods() > 0) {
- unsigned DiagID = isa<CXXDestructorDecl>(MD)
- ? diag::warn_destructor_marked_not_override_overriding
- : diag::warn_function_marked_not_override_overriding;
- Diag(MD->getLocation(), DiagID) << MD->getDeclName();
- const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
- Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+ auto EmitDiag = [this, MD](unsigned DiagID) {
+ Diag(MD->getLocation(), DiagID) << MD->getDeclName();
+ const CXXMethodDecl *OMD = *MD->begin_overridden_methods();
+ Diag(OMD->getLocation(), diag::note_overridden_virtual_function);
+ };
+ if (Inconsistent)
+ EmitDiag(isa<CXXDestructorDecl>(MD)
+ ? diag::warn_inconsistent_destructor_marked_not_override_overriding
+ : diag::warn_inconsistent_function_marked_not_override_overriding);
+ else
+ EmitDiag(isa<CXXDestructorDecl>(MD)
+ ? diag::warn_suggest_destructor_marked_not_override_overriding
+ : diag::warn_suggest_function_marked_not_override_overriding);
}
}
@@ -6749,13 +6756,10 @@
}
}
- if (HasMethodWithOverrideControl &&
- HasOverridingMethodWithoutOverrideControl) {
- // At least one method has the 'override' control declared.
- // Diagnose all other overridden methods which do not have 'override'
- // specified on them.
+ if (HasOverridingMethodWithoutOverrideControl) {
+ bool InconsistentOverrideControl = HasMethodWithOverrideControl;
for (auto *M : Record->methods())
- DiagnoseAbsenceOfOverrideControl(M);
+ DiagnoseAbsenceOfOverrideControl(M, InconsistentOverrideControl);
}
// Check the defaulted secondary comparisons after any other member functions.
Index: clang/include/clang/Sema/Sema.h
===================================================================
--- clang/include/clang/Sema/Sema.h
+++ clang/include/clang/Sema/Sema.h
@@ -6930,7 +6930,7 @@
/// DiagnoseAbsenceOfOverrideControl - Diagnose if 'override' keyword was
/// not used in the declaration of an overriding method.
- void DiagnoseAbsenceOfOverrideControl(NamedDecl *D);
+ void DiagnoseAbsenceOfOverrideControl(NamedDecl *D, bool Inconsistent);
/// CheckForFunctionMarkedFinal - Checks whether a virtual member function
/// overrides a virtual member function marked 'final', according to
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -2361,12 +2361,22 @@
"%select{function|functions}1">;
def err_function_marked_override_not_overriding : Error<
"%0 marked 'override' but does not override any member functions">;
-def warn_destructor_marked_not_override_overriding : Warning <
- "%0 overrides a destructor but is not marked 'override'">,
- InGroup<CXX11WarnOverrideDestructor>, DefaultIgnore;
-def warn_function_marked_not_override_overriding : Warning <
- "%0 overrides a member function but is not marked 'override'">,
- InGroup<CXX11WarnOverrideMethod>;
+def warn_destructor_marked_not_override_overriding : TextSubstitution <
+ "%0 overrides a destructor but is not marked 'override'">;
+def warn_function_marked_not_override_overriding : TextSubstitution <
+ "%0 overrides a member function but is not marked 'override'">;
+def warn_inconsistent_destructor_marked_not_override_overriding : Warning <
+ "%sub{warn_destructor_marked_not_override_overriding}0">,
+ InGroup<CXX11WarnInconsistentOverrideDestructor>, DefaultIgnore;
+def warn_inconsistent_function_marked_not_override_overriding : Warning <
+ "%sub{warn_function_marked_not_override_overriding}0">,
+ InGroup<CXX11WarnInconsistentOverrideMethod>;
+def warn_suggest_destructor_marked_not_override_overriding : Warning <
+ "%sub{warn_destructor_marked_not_override_overriding}0">,
+ InGroup<CXX11WarnSuggestOverrideDestructor>, DefaultIgnore;
+def warn_suggest_function_marked_not_override_overriding : Warning <
+ "%sub{warn_function_marked_not_override_overriding}0">,
+ InGroup<CXX11WarnSuggestOverride>, DefaultIgnore;
def err_class_marked_final_used_as_base : Error<
"base %0 is marked '%select{final|sealed}1'">;
def warn_abstract_final_class : Warning<
Index: clang/include/clang/Basic/DiagnosticGroups.td
===================================================================
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -279,9 +279,13 @@
def CXX11Narrowing : DiagGroup<"c++11-narrowing">;
-def CXX11WarnOverrideDestructor :
+def CXX11WarnInconsistentOverrideDestructor :
DiagGroup<"inconsistent-missing-destructor-override">;
-def CXX11WarnOverrideMethod : DiagGroup<"inconsistent-missing-override">;
+def CXX11WarnInconsistentOverrideMethod :
+ DiagGroup<"inconsistent-missing-override">;
+def CXX11WarnSuggestOverrideDestructor : DiagGroup<"suggest-destructor-override">;
+def CXX11WarnSuggestOverride : DiagGroup<"suggest-override",
+ [CXX11WarnSuggestOverrideDestructor]>;
// Original name of this warning in Clang
def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>;
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
