On Fri, Oct 3, 2014 at 4:24 AM, Richard Smith <[email protected]> wrote: > On Thu, Oct 2, 2014 at 4:13 PM, Fariborz Jahanian <[email protected]> > wrote: >> >> Author: fjahanian >> Date: Thu Oct 2 18:13:51 2014 >> New Revision: 218925 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=218925&view=rev >> Log: >> Patch to warn if 'override' is missing >> for an overriding method if class has at least one >> 'override' specified on one of its methods. >> Reviewed by Doug Gregor. rdar://18295240 >> (I have already checked in all llvm files with missing 'override' >> methods and Bob Wilson has fixed a TableGen of FastISel so >> no warnings are expected from build of llvm after this patch. >> I have already verified this). >> >> Added: >> cfe/trunk/test/FixIt/cxx11-fixit-missing-override.cpp >> cfe/trunk/test/SemaCXX/cxx11-warn-missing-override.cpp >> Modified: >> cfe/trunk/include/clang/Basic/DiagnosticGroups.td >> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> cfe/trunk/include/clang/Sema/Sema.h >> cfe/trunk/lib/Sema/SemaDeclCXX.cpp >> cfe/trunk/test/CXX/class.derived/class.virtual/p3-0x.cpp >> cfe/trunk/test/FixIt/fixit-cxx0x.cpp >> cfe/trunk/test/Parser/MicrosoftExtensions.cpp >> cfe/trunk/test/Parser/cxx0x-decl.cpp >> cfe/trunk/test/Parser/cxx0x-in-cxx98.cpp >> cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp >> cfe/trunk/test/SemaCXX/attr-gnu.cpp >> cfe/trunk/test/SemaCXX/cxx98-compat.cpp >> cfe/trunk/test/SemaCXX/ms-interface.cpp >> >> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original) >> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Thu Oct 2 18:13:51 >> 2014 >> @@ -146,6 +146,8 @@ def CXX98CompatPedantic : DiagGroup<"c++ >> >> def CXX11Narrowing : DiagGroup<"c++11-narrowing">; >> >> +def CXX11WarnOverrideMethod : DiagGroup<"inconsistent-missing-override">; >> + >> // Original name of this warning in Clang >> def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>; >> >> >> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) >> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Oct 2 >> 18:13:51 2014 >> @@ -1689,6 +1689,9 @@ def override_keyword_hides_virtual_membe >> "%select{function|functions}1">; >> def err_function_marked_override_not_overriding : Error< >> "%0 marked 'override' but does not override any member functions">; >> +def warn_function_marked_not_override_overriding : Warning < >> + "%0 overrides a member function but is not marked 'override'">, >> + InGroup<CXX11WarnOverrideMethod>; >> def err_class_marked_final_used_as_base : Error< >> "base %0 is marked '%select{final|sealed}1'">; >> def warn_abstract_final_class : Warning< >> >> Modified: cfe/trunk/include/clang/Sema/Sema.h >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/include/clang/Sema/Sema.h (original) >> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Oct 2 18:13:51 2014 >> @@ -5084,6 +5084,10 @@ public: >> >> /// CheckOverrideControl - Check C++11 override control semantics. >> void CheckOverrideControl(NamedDecl *D); >> + >> + /// DiagnoseAbsenseOfOverrideControl - Diagnose if override control was >> + /// not used in the; otherwise, overriding method. > > > Something weird has happened to this comment. > >> >> + void DiagnoseAbsenseOfOverrideControl(NamedDecl *D); >> >> /// CheckForFunctionMarkedFinal - Checks whether a virtual member >> function >> /// overrides a virtual member function marked 'final', according to >> >> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Thu Oct 2 18:13:51 2014 >> @@ -1897,6 +1897,31 @@ void Sema::CheckOverrideControl(NamedDec >> << MD->getDeclName(); >> } >> >> +void Sema::DiagnoseAbsenseOfOverrideControl(NamedDecl *D) { >> + if (D->isInvalidDecl()) >> + return; >> + CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D); >> + if (!MD || MD->isImplicit() || isa<CXXDestructorDecl>(MD)) >> + return; > > > Why do you skip destructors? > >> + bool HasOverriddenMethods = >> + MD->begin_overridden_methods() != MD->end_overridden_methods(); >> + if (HasOverriddenMethods) { >> + SourceLocation EndLocation = >> + (MD->isPure() || MD->hasAttr<FinalAttr>()) > > > We should not warn if the function (or the class) has the 'final' attribute.
And it breaks bootstrap: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/13183/steps/bootstrap%20clang/logs/stdio If we choose to warn about missing override on final methods, we should at least fix all such cases in LLVM code. >> >> + ? SourceLocation() : MD->getSourceRange().getEnd(); >> + Diag(MD->getLocation(), >> diag::warn_function_marked_not_override_overriding) >> + << MD->getDeclName() >> + << FixItHint::CreateReplacement(EndLocation, ") override"); > > > This fixit is not correct. The EndLocation isn't necessarily the location of > the `)`, and the `override` does not necessarily go after the `)`. (Consider > cv-qualifiers and ref-qualifiers, or functions whose return type includes > trailing declarator chunks, or functions with trailing return types.) > >> >> + for (CXXMethodDecl::method_iterator I = >> MD->begin_overridden_methods(), >> + E = MD->end_overridden_methods(); I != E; ++I) { >> + const CXXMethodDecl *OMD = *I; >> + Diag(OMD->getLocation(), diag::note_overridden_virtual_function); >> + break; >> + } >> + } >> +} >> + >> /// CheckIfOverriddenFunctionIsMarkedFinal - Checks whether a virtual >> member >> /// function overrides a virtual member function marked 'final', >> according to >> /// C++11 [class.virtual]p4. >> @@ -4680,13 +4705,18 @@ void Sema::CheckCompletedCXXClass(CXXRec >> } >> } >> >> + bool HasMethodWithOverrideControl = false, >> + HasOverridingMethodWithoutOverrideControl = false; >> if (!Record->isDependentType()) { >> for (auto *M : Record->methods()) { >> // See if a method overloads virtual methods in a base >> // class without overriding any. >> if (!M->isStatic()) >> DiagnoseHiddenVirtualMethods(M); >> - >> + if (M->hasAttr<OverrideAttr>()) >> + HasMethodWithOverrideControl = true; >> + else if (M->begin_overridden_methods() != >> M->end_overridden_methods()) > > > Maybe just use M->size_overridden_methods() here. > >> >> + HasOverridingMethodWithoutOverrideControl = true; >> // Check whether the explicitly-defaulted special members are >> valid. >> if (!M->isInvalidDecl() && M->isExplicitlyDefaulted()) >> CheckExplicitlyDefaultedSpecialMember(M); >> @@ -4705,6 +4735,14 @@ void Sema::CheckCompletedCXXClass(CXXRec >> } >> } >> >> + 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. >> + for (auto *M : Record->methods()) >> + if (!M->hasAttr<OverrideAttr>()) >> + DiagnoseAbsenseOfOverrideControl(M); >> + } >> // C++11 [dcl.constexpr]p8: A constexpr specifier for a non-static >> member >> // function that is not a constructor declares that member function to >> be >> // const. [...] The class of which that function is a member shall be >> >> Modified: cfe/trunk/test/CXX/class.derived/class.virtual/p3-0x.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/class.derived/class.virtual/p3-0x.cpp?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/CXX/class.derived/class.virtual/p3-0x.cpp (original) >> +++ cfe/trunk/test/CXX/class.derived/class.virtual/p3-0x.cpp Thu Oct 2 >> 18:13:51 2014 >> @@ -61,7 +61,7 @@ struct D : B { >> namespace PR13499 { >> struct X { >> virtual void f(); >> - virtual void h(); >> + virtual void h(); // expected-note 2 {{overridden virtual function is >> here}} >> }; >> template<typename T> struct A : X { >> void f() override; >> @@ -83,7 +83,7 @@ namespace PR13499 { >> template<typename...T> struct E : X { >> void f(T...) override; >> void g(T...) override; // expected-error {{only virtual member >> functions can be marked 'override'}} >> - void h(T...) final; >> + void h(T...) final; // expected-warning {{'h' overrides a member >> function but is not marked 'override'}} > > > This warning is bogus; it makes no sense to require both 'override' and > 'final'. > >> void i(T...) final; // expected-error {{only virtual member functions >> can be marked 'final'}} >> }; >> // FIXME: Diagnose these in the template definition, not in the >> instantiation. >> @@ -91,13 +91,13 @@ namespace PR13499 { >> >> template<typename T> struct Y : T { >> void f() override; >> - void h() final; >> + void h() final; // expected-warning {{'h' overrides a member function >> but is not marked 'override'}} > > > ... likewise. > >> >> }; >> template<typename T> struct Z : T { >> void g() override; // expected-error {{only virtual member functions >> can be marked 'override'}} >> void i() final; // expected-error {{only virtual member functions can >> be marked 'final'}} >> }; >> - Y<X> y; >> + Y<X> y; // expected-note {{in instantiation of}} >> Z<X> z; // expected-note {{in instantiation of}} >> } >> >> >> Added: cfe/trunk/test/FixIt/cxx11-fixit-missing-override.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/FixIt/cxx11-fixit-missing-override.cpp?rev=218925&view=auto >> >> ============================================================================== >> --- cfe/trunk/test/FixIt/cxx11-fixit-missing-override.cpp (added) >> +++ cfe/trunk/test/FixIt/cxx11-fixit-missing-override.cpp Thu Oct 2 >> 18:13:51 2014 >> @@ -0,0 +1,33 @@ >> +// RUN: cp %s %t >> +// RUN: %clang_cc1 -x c++ -std=c++11 -Winconsistent-missing-override >> -fixit %t >> +// RUN: %clang_cc1 -x c++ -std=c++11 -Winconsistent-missing-override >> -Werror %t >> + >> +struct A >> +{ >> + virtual void foo(); >> + virtual void bar(); // expected-note {{overridden virtual function is >> here}} >> + virtual void gorf() {} >> + virtual void g() = 0; // expected-note {{overridden virtual function >> is here}} >> +}; >> + >> +struct B : A >> +{ >> + void foo() override; >> + void bar(); // expected-warning {{'bar' overrides a member function >> but is not marked 'override'}} >> +}; >> + >> +struct C : B >> +{ >> + virtual void g() override = 0; // expected-warning {{'g' overrides a >> member function but is not marked 'override'}} >> + virtual void gorf() override {} >> + void foo() {} >> +}; >> + >> +struct D : C { >> + virtual void g()override ; >> + virtual void foo(){ >> + } >> + void bar() override; >> +}; >> + >> + >> >> Modified: cfe/trunk/test/FixIt/fixit-cxx0x.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/FixIt/fixit-cxx0x.cpp?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/FixIt/fixit-cxx0x.cpp (original) >> +++ cfe/trunk/test/FixIt/fixit-cxx0x.cpp Thu Oct 2 18:13:51 2014 >> @@ -24,7 +24,7 @@ namespace SemiCommaTypo { >> int o; >> >> struct Base { >> - virtual void f2(), f3(); >> + virtual void f2(), f3(); // expected-note {{overridden virtual >> function is here}} >> }; >> struct MemberDeclarator : Base { >> int k : 4, >> @@ -33,7 +33,7 @@ namespace SemiCommaTypo { >> char c, // expected-error {{expected ';' at end of declaration}} >> typedef void F(), // expected-error {{expected ';' at end of >> declaration}} >> F f1, >> - f2 final, >> + f2 final, // expected-warning {{'f2' overrides a member function >> but is not marked 'override'}} >> f3 override, // expected-error {{expected ';' at end of >> declaration}} >> }; >> } >> >> Modified: cfe/trunk/test/Parser/MicrosoftExtensions.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/MicrosoftExtensions.cpp?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/Parser/MicrosoftExtensions.cpp (original) >> +++ cfe/trunk/test/Parser/MicrosoftExtensions.cpp Thu Oct 2 18:13:51 2014 >> @@ -208,12 +208,12 @@ extern TypenameWrongPlace<AAAA> PR16925; >> >> __interface MicrosoftInterface; >> __interface MicrosoftInterface { >> - void foo1() = 0; >> + void foo1() = 0; // expected-note {{overridden virtual function is >> here}} >> virtual void foo2() = 0; >> }; >> >> __interface MicrosoftDerivedInterface : public MicrosoftInterface { >> - void foo1(); >> + void foo1(); // expected-warning {{'foo1' overrides a member function >> but is not marked 'override'}} >> void foo2() override; >> void foo3(); >> }; >> >> Modified: cfe/trunk/test/Parser/cxx0x-decl.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/cxx0x-decl.cpp?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/Parser/cxx0x-decl.cpp (original) >> +++ cfe/trunk/test/Parser/cxx0x-decl.cpp Thu Oct 2 18:13:51 2014 >> @@ -83,13 +83,13 @@ namespace PR5066 { >> >> namespace FinalOverride { >> struct Base { >> - virtual void *f(); >> + virtual void *f(); // expected-note {{overridden virtual function is >> here}} >> virtual void *g(); >> virtual void *h(); >> virtual void *i(); >> }; >> struct Derived : Base { >> - virtual auto f() -> void *final; >> + virtual auto f() -> void *final; // expected-warning {{'f' overrides >> a member function but is not marked 'override'}} >> virtual auto g() -> void *override; >> virtual auto h() -> void *final override; >> virtual auto i() -> void *override final; >> >> Modified: cfe/trunk/test/Parser/cxx0x-in-cxx98.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/cxx0x-in-cxx98.cpp?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/Parser/cxx0x-in-cxx98.cpp (original) >> +++ cfe/trunk/test/Parser/cxx0x-in-cxx98.cpp Thu Oct 2 18:13:51 2014 >> @@ -10,11 +10,12 @@ struct X { >> >> struct B { >> virtual void f(); >> - virtual void g(); >> + virtual void g(); // expected-note {{overridden virtual function is >> here}} >> }; >> struct D final : B { // expected-warning {{'final' keyword is a C++11 >> extension}} >> virtual void f() override; // expected-warning {{'override' keyword is >> a C++11 extension}} >> - virtual void g() final; // expected-warning {{'final' keyword is a >> C++11 extension}} >> + virtual void g() final; // expected-warning {{'final' keyword is a >> C++11 extension}} \ >> + // expected-warning {{'g' overrides a member >> function but is not marked 'override'}} >> }; >> >> void NewBracedInitList() { >> >> Modified: cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp (original) >> +++ cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp Thu Oct 2 18:13:51 >> 2014 >> @@ -372,14 +372,14 @@ struct SomeBase { >> >> // expected-note@+2 {{overridden virtual function is here}} >> // expected-warning@+1 {{'sealed' keyword is a Microsoft extension}} >> - virtual void SealedFunction() sealed; >> + virtual void SealedFunction() sealed; // expected-note {{overridden >> virtual function is here}} >> }; >> >> // expected-note@+2 {{'SealedType' declared here}} >> // expected-warning@+1 {{'sealed' keyword is a Microsoft extension}} >> struct SealedType sealed : SomeBase { >> // expected-error@+1 {{declaration of 'SealedFunction' overrides a >> 'sealed' function}} >> - virtual void SealedFunction(); >> + virtual void SealedFunction(); // expected-warning {{'SealedFunction' >> overrides a member function but is not marked 'override'}} >> >> // expected-warning@+1 {{'override' keyword is a C++11 extension}} >> virtual void OverrideMe() override; >> >> Modified: cfe/trunk/test/SemaCXX/attr-gnu.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-gnu.cpp?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/SemaCXX/attr-gnu.cpp (original) >> +++ cfe/trunk/test/SemaCXX/attr-gnu.cpp Thu Oct 2 18:13:51 2014 >> @@ -15,14 +15,15 @@ void g(int a[static [[]] 5]); // expecte >> namespace { >> class B { >> public: >> - virtual void test() {} >> + virtual void test() {} // expected-note {{overridden virtual function >> is here}} >> virtual void test2() {} >> virtual void test3() {} >> }; >> >> class D : public B { >> public: >> - void test() __attribute__((deprecated)) final {} // expected-warning >> {{GCC does not allow an attribute in this position on a function >> declaration}} >> + void test() __attribute__((deprecated)) final {} // expected-warning >> {{GCC does not allow an attribute in this position on a function >> declaration}} \ >> + // expected-warning >> {{'test' overrides a member function but is not marked 'override'}} >> void test2() [[]] override {} // Ok >> void test3() __attribute__((cf_unknown_transfer)) override {} // Ok, >> not known to GCC. >> }; >> >> Added: cfe/trunk/test/SemaCXX/cxx11-warn-missing-override.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/cxx11-warn-missing-override.cpp?rev=218925&view=auto >> >> ============================================================================== >> --- cfe/trunk/test/SemaCXX/cxx11-warn-missing-override.cpp (added) >> +++ cfe/trunk/test/SemaCXX/cxx11-warn-missing-override.cpp Thu Oct 2 >> 18:13:51 2014 >> @@ -0,0 +1,21 @@ >> +// RUN: %clang_cc1 -fsyntax-only -Winconsistent-missing-override -verify >> -std=c++11 %s >> +struct A >> +{ >> + virtual void foo(); >> + virtual void bar(); // expected-note {{overridden virtual function is >> here}} >> + virtual void gorf() {} >> + virtual void g() = 0; // expected-note {{overridden virtual function >> is here}} >> +}; >> + >> +struct B : A >> +{ >> + void foo() override; >> + void bar(); // expected-warning {{'bar' overrides a member function >> but is not marked 'override'}} >> +}; >> + >> +struct C : B >> +{ >> + virtual void g() = 0; // expected-warning {{'g' overrides a member >> function but is not marked 'override'}} >> + virtual void gorf() override {} >> +}; >> + >> >> Modified: cfe/trunk/test/SemaCXX/cxx98-compat.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/cxx98-compat.cpp?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/SemaCXX/cxx98-compat.cpp (original) >> +++ cfe/trunk/test/SemaCXX/cxx98-compat.cpp Thu Oct 2 18:13:51 2014 >> @@ -120,11 +120,12 @@ struct InClassInit { >> >> struct OverrideControlBase { >> virtual void f(); >> - virtual void g(); >> + virtual void g(); // expected-note {{overridden virtual function is >> here}} >> }; >> struct OverrideControl final : OverrideControlBase { // expected-warning >> {{'final' keyword is incompatible with C++98}} >> virtual void f() override; // expected-warning {{'override' keyword is >> incompatible with C++98}} >> - virtual void g() final; // expected-warning {{'final' keyword is >> incompatible with C++98}} >> + virtual void g() final; // expected-warning {{'final' keyword is >> incompatible with C++98}} \ >> + // expected-warning {{'g' overrides a member >> function but is not marked 'override'}} >> }; >> >> using AliasDecl = int; // expected-warning {{alias declarations are >> incompatible with C++98}} >> >> Modified: cfe/trunk/test/SemaCXX/ms-interface.cpp >> URL: >> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/ms-interface.cpp?rev=218925&r1=218924&r2=218925&view=diff >> >> ============================================================================== >> --- cfe/trunk/test/SemaCXX/ms-interface.cpp (original) >> +++ cfe/trunk/test/SemaCXX/ms-interface.cpp Thu Oct 2 18:13:51 2014 >> @@ -12,7 +12,7 @@ __interface I1 { >> operator int(); >> // expected-error@+1 {{nested class I1::(anonymous) is not permitted >> within an interface type}} >> struct { int a; }; >> - void fn2() { >> + void fn2() { // expected-note {{overridden virtual function is here}} >> struct A { }; // should be ignored: not a nested class >> } >> protected: // expected-error {{interface types cannot specify 'protected' >> access}} >> @@ -44,7 +44,7 @@ __interface I3 final { >> __interface I4 : I1, I2 { >> void fn1() const override; >> // expected-error@+1 {{'final' keyword not permitted with interface >> types}} >> - void fn2() final; >> + void fn2() final; // expected-warning {{'fn2' overrides a member >> function but is not marked 'override'}} >> }; >> >> // expected-error@+1 {{interface type cannot inherit from non-public >> 'interface I1'}} >> >> >> _______________________________________________ >> cfe-commits mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
