Author: Logan Smith Date: 2020-07-12T16:05:24-07:00 New Revision: 111167895d47558989f9f3a593a82527b016c7e7
URL: https://github.com/llvm/llvm-project/commit/111167895d47558989f9f3a593a82527b016c7e7 DIFF: https://github.com/llvm/llvm-project/commit/111167895d47558989f9f3a593a82527b016c7e7.diff LOG: [clang] Add -Wsuggest-override 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. Reviewed By: dblaikie Differential Revision: https://reviews.llvm.org/D82728 Added: clang/test/SemaCXX/warn-suggest-destructor-override clang/test/SemaCXX/warn-suggest-override Modified: clang/include/clang/Basic/DiagnosticGroups.td clang/include/clang/Basic/DiagnosticSemaKinds.td clang/include/clang/Sema/Sema.h clang/lib/Sema/SemaDeclCXX.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 6a50ceef4191..1e829be4028e 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -280,9 +280,12 @@ def CXX98CompatPedantic : DiagGroup<"c++98-compat-pedantic", 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">; // 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 24e942037ecf..71517edd6659 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -2367,12 +2367,22 @@ def override_keyword_hides_virtual_member_function : Error< "%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< diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index e75ac185eb2c..6f7ad8076718 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -6965,7 +6965,7 @@ class Sema final { /// 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 diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp index 9cad6debc600..515a2e9690ed 100644 --- a/clang/lib/Sema/SemaDeclCXX.cpp +++ b/clang/lib/Sema/SemaDeclCXX.cpp @@ -3045,7 +3045,7 @@ void Sema::CheckOverrideControl(NamedDecl *D) { << 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,22 @@ void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) { 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 = [&](unsigned DiagInconsistent, unsigned DiagSuggest) { + unsigned DiagID = + Inconsistent && !Diags.isIgnored(DiagInconsistent, MD->getLocation()) + ? DiagInconsistent + : DiagSuggest; + Diag(MD->getLocation(), DiagID) << MD->getDeclName(); + const CXXMethodDecl *OMD = *MD->begin_overridden_methods(); + Diag(OMD->getLocation(), diag::note_overridden_virtual_function); + }; + if (isa<CXXDestructorDecl>(MD)) + EmitDiag( + diag::warn_inconsistent_destructor_marked_not_override_overriding, + diag::warn_suggest_destructor_marked_not_override_overriding); + else + EmitDiag(diag::warn_inconsistent_function_marked_not_override_overriding, + diag::warn_suggest_function_marked_not_override_overriding); } } @@ -6749,13 +6759,10 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) { } } - 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 HasInconsistentOverrideControl = HasMethodWithOverrideControl; for (auto *M : Record->methods()) - DiagnoseAbsenceOfOverrideControl(M); + DiagnoseAbsenceOfOverrideControl(M, HasInconsistentOverrideControl); } // Check the defaulted secondary comparisons after any other member functions. diff --git a/clang/test/SemaCXX/warn-suggest-destructor-override b/clang/test/SemaCXX/warn-suggest-destructor-override new file mode 100644 index 000000000000..1cfff748678f --- /dev/null +++ b/clang/test/SemaCXX/warn-suggest-destructor-override @@ -0,0 +1,27 @@ +// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-destructor-override + +struct A { + ~A(); + virtual void run(); +}; + +struct B : public A { + ~B(); +}; + +struct C { + virtual void run(); + virtual ~C(); // expected-note 2{{overridden virtual function is here}} +}; + +struct D : public C { + void run(); + ~D(); + // expected-warning@-1 {{'~D' overrides a destructor but is not marked 'override'}} +}; + +struct E : public C { + void run(); + virtual ~E(); + // expected-warning@-1 {{'~E' overrides a destructor but is not marked 'override'}} +}; diff --git a/clang/test/SemaCXX/warn-suggest-override b/clang/test/SemaCXX/warn-suggest-override new file mode 100644 index 000000000000..e06c939ff001 --- /dev/null +++ b/clang/test/SemaCXX/warn-suggest-override @@ -0,0 +1,38 @@ +// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify -Wsuggest-override + +struct A { + ~A(); + void run(); +}; + +struct B : public A { + ~B(); + void run(); +}; + +struct C { + virtual void run(); // expected-note 2{{overridden virtual function is here}} + virtual ~C(); +}; + +struct D : public C { + void run(); + // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}} + ~D(); +}; + +struct E : public C { + virtual void run(); + // expected-warning@-1 {{'run()' overrides a member function but is not marked 'override'}} + virtual ~E(); +}; + +struct F : public C { + void run() override; + ~F() override; +}; + +struct G : public C { + void run() final; + ~G() final; +}; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
