I've now looked at a bit over 1000 instances of this warning (and fixed many of them); with this experience I have two more pieces of feedback:
1. This is a very cool warning. 2. In practice, the methods this warns on sometimes comes from a macro and it's impossible to add override. Examples are gmock's MOCK_METHODn, and IFACEMETHOD and COM_INTERFACE_ENTRY from the Windows SDK. Maybe this shouldn't warn if the method it warns on comes from a macro expansion? Or, if you think that that would hide too many issues (it did find a few cases where I could just add "override" to a macro, in my experience), maybe at least if the macro that's expanded is from a system header? Thanks, Nico On Mon, Oct 27, 2014 at 12:11 PM, Fariborz Jahanian <[email protected]> wrote: > Author: fjahanian > Date: Mon Oct 27 14:11:51 2014 > New Revision: 220703 > > URL: http://llvm.org/viewvc/llvm-project?rev=220703&view=rev > Log: > c++11 patch to issue warning on missing 'override' on > overriding methods. Patch review by Richard Smith. > rdar://18295240 > > 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/Parser/MicrosoftExtensions.cpp > cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp > > Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=220703&r1=220702&r2=220703&view=diff > > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Mon Oct 27 14:11:51 > 2014 > @@ -147,6 +147,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=220703&r1=220702&r2=220703&view=diff > > ============================================================================== > --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original) > +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Oct 27 > 14:11:51 2014 > @@ -1691,6 +1691,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=220703&r1=220702&r2=220703&view=diff > > ============================================================================== > --- cfe/trunk/include/clang/Sema/Sema.h (original) > +++ cfe/trunk/include/clang/Sema/Sema.h Mon Oct 27 14:11:51 2014 > @@ -5138,6 +5138,10 @@ public: > > /// CheckOverrideControl - Check C++11 override control semantics. > void CheckOverrideControl(NamedDecl *D); > + > + /// DiagnoseAbsenceOfOverrideControl - Diagnose if 'override' keyword > was > + /// not used in the declaration of an overriding method. > + void DiagnoseAbsenceOfOverrideControl(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=220703&r1=220702&r2=220703&view=diff > > ============================================================================== > --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original) > +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Oct 27 14:11:51 2014 > @@ -1897,6 +1897,22 @@ void Sema::CheckOverrideControl(NamedDec > << MD->getDeclName(); > } > > +void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) { > + if (D->isInvalidDecl() || D->hasAttr<OverrideAttr>()) > + return; > + CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D); > + if (!MD || MD->isImplicit() || MD->hasAttr<FinalAttr>() || > + isa<CXXDestructorDecl>(MD)) > + return; > + > + if (MD->size_overridden_methods() > 0) { > + Diag(MD->getLocation(), > diag::warn_function_marked_not_override_overriding) > + << MD->getDeclName(); > + const CXXMethodDecl *OMD = *MD->begin_overridden_methods(); > + Diag(OMD->getLocation(), diag::note_overridden_virtual_function); > + } > +} > + > /// CheckIfOverriddenFunctionIsMarkedFinal - Checks whether a virtual > member > /// function overrides a virtual member function marked 'final', > according to > /// C++11 [class.virtual]p4. > @@ -4763,13 +4779,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->size_overridden_methods() > 0) > + HasOverridingMethodWithoutOverrideControl = true; > // Check whether the explicitly-defaulted special members are valid. > if (!M->isInvalidDecl() && M->isExplicitlyDefaulted()) > CheckExplicitlyDefaultedSpecialMember(M); > @@ -4788,6 +4809,13 @@ 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()) > + DiagnoseAbsenceOfOverrideControl(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/Parser/MicrosoftExtensions.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/MicrosoftExtensions.cpp?rev=220703&r1=220702&r2=220703&view=diff > > ============================================================================== > --- cfe/trunk/test/Parser/MicrosoftExtensions.cpp (original) > +++ cfe/trunk/test/Parser/MicrosoftExtensions.cpp Mon Oct 27 14:11: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/SemaCXX/MicrosoftExtensions.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp?rev=220703&r1=220702&r2=220703&view=diff > > ============================================================================== > --- cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp (original) > +++ cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp Mon Oct 27 14:11:51 2014 > @@ -372,14 +372,15 @@ 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(); > + // expected-error@+2 {{declaration of 'SealedFunction' overrides a > 'sealed' function}} > + // FIXME. warning can be suppressed if we're also issuing error for > overriding a 'final' function. > + 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; > > > _______________________________________________ > 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
