r220703 - c++11 patch to issue warning on missing 'override' on
Nico Weber
thakis at chromium.org
Wed Oct 29 18:23:10 PDT 2014
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 <fjahanian at apple.com>
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 at +2 {{overridden virtual function is here}}
> // expected-warning at +1 {{'sealed' keyword is a Microsoft extension}}
> - virtual void SealedFunction() sealed;
> + virtual void SealedFunction() sealed; // expected-note {{overridden
> virtual function is here}}
> };
>
> // expected-note at +2 {{'SealedType' declared here}}
> // expected-warning at +1 {{'sealed' keyword is a Microsoft extension}}
> struct SealedType sealed : SomeBase {
> - // expected-error at +1 {{declaration of 'SealedFunction' overrides a
> 'sealed' function}}
> - virtual void SealedFunction();
> + // expected-error at +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 at +1 {{'override' keyword is a C++11 extension}}
> virtual void OverrideMe() override;
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141029/5afc712d/attachment.html>
More information about the cfe-commits
mailing list