<div dir="ltr">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:<div><br></div><div>1. This is a very cool warning.</div><div>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 <span style="color:rgb(0,0,0);font-size:12px;white-space:pre-wrap">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?</span></div><div><span style="color:rgb(0,0,0);font-size:12px;white-space:pre-wrap"><br></span></div><div><span style="color:rgb(0,0,0);font-size:12px;white-space:pre-wrap">Thanks,</span></div><div><span style="color:rgb(0,0,0);font-size:12px;white-space:pre-wrap">Nico</span></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 27, 2014 at 12:11 PM, Fariborz Jahanian <span dir="ltr"><<a href="mailto:fjahanian@apple.com" target="_blank">fjahanian@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: fjahanian<br>
Date: Mon Oct 27 14:11:51 2014<br>
New Revision: 220703<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=220703&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=220703&view=rev</a><br>
Log:<br>
c++11 patch to issue warning on missing 'override' on<br>
overriding methods. Patch review by Richard Smith.<br>
rdar://18295240<br>
<br>
Modified:<br>
cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
cfe/trunk/include/clang/Sema/Sema.h<br>
cfe/trunk/lib/Sema/SemaDeclCXX.cpp<br>
cfe/trunk/test/Parser/MicrosoftExtensions.cpp<br>
cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=220703&r1=220702&r2=220703&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=220703&r1=220702&r2=220703&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Mon Oct 27 14:11:51 2014<br>
@@ -147,6 +147,8 @@ def CXX98CompatPedantic : DiagGroup<"c++<br>
<br>
def CXX11Narrowing : DiagGroup<"c++11-narrowing">;<br>
<br>
+def CXX11WarnOverrideMethod : DiagGroup<"inconsistent-missing-override">;<br>
+<br>
// Original name of this warning in Clang<br>
def : DiagGroup<"c++0x-narrowing", [CXX11Narrowing]>;<br>
<br>
<br>
Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=220703&r1=220702&r2=220703&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=220703&r1=220702&r2=220703&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)<br>
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Oct 27 14:11:51 2014<br>
@@ -1691,6 +1691,9 @@ def override_keyword_hides_virtual_membe<br>
"%select{function|functions}1">;<br>
def err_function_marked_override_not_overriding : Error<<br>
"%0 marked 'override' but does not override any member functions">;<br>
+def warn_function_marked_not_override_overriding : Warning <<br>
+ "%0 overrides a member function but is not marked 'override'">,<br>
+ InGroup<CXX11WarnOverrideMethod>;<br>
def err_class_marked_final_used_as_base : Error<<br>
"base %0 is marked '%select{final|sealed}1'">;<br>
def warn_abstract_final_class : Warning<<br>
<br>
Modified: cfe/trunk/include/clang/Sema/Sema.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=220703&r1=220702&r2=220703&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=220703&r1=220702&r2=220703&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/include/clang/Sema/Sema.h (original)<br>
+++ cfe/trunk/include/clang/Sema/Sema.h Mon Oct 27 14:11:51 2014<br>
@@ -5138,6 +5138,10 @@ public:<br>
<br>
/// CheckOverrideControl - Check C++11 override control semantics.<br>
void CheckOverrideControl(NamedDecl *D);<br>
+<br>
+ /// DiagnoseAbsenceOfOverrideControl - Diagnose if 'override' keyword was<br>
+ /// not used in the declaration of an overriding method.<br>
+ void DiagnoseAbsenceOfOverrideControl(NamedDecl *D);<br>
<br>
/// CheckForFunctionMarkedFinal - Checks whether a virtual member function<br>
/// overrides a virtual member function marked 'final', according to<br>
<br>
Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=220703&r1=220702&r2=220703&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=220703&r1=220702&r2=220703&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)<br>
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Oct 27 14:11:51 2014<br>
@@ -1897,6 +1897,22 @@ void Sema::CheckOverrideControl(NamedDec<br>
<< MD->getDeclName();<br>
}<br>
<br>
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {<br>
+ if (D->isInvalidDecl() || D->hasAttr<OverrideAttr>())<br>
+ return;<br>
+ CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D);<br>
+ if (!MD || MD->isImplicit() || MD->hasAttr<FinalAttr>() ||<br>
+ isa<CXXDestructorDecl>(MD))<br>
+ return;<br>
+<br>
+ if (MD->size_overridden_methods() > 0) {<br>
+ Diag(MD->getLocation(), diag::warn_function_marked_not_override_overriding)<br>
+ << MD->getDeclName();<br>
+ const CXXMethodDecl *OMD = *MD->begin_overridden_methods();<br>
+ Diag(OMD->getLocation(), diag::note_overridden_virtual_function);<br>
+ }<br>
+}<br>
+<br>
/// CheckIfOverriddenFunctionIsMarkedFinal - Checks whether a virtual member<br>
/// function overrides a virtual member function marked 'final', according to<br>
/// C++11 [class.virtual]p4.<br>
@@ -4763,13 +4779,18 @@ void Sema::CheckCompletedCXXClass(CXXRec<br>
}<br>
}<br>
<br>
+ bool HasMethodWithOverrideControl = false,<br>
+ HasOverridingMethodWithoutOverrideControl = false;<br>
if (!Record->isDependentType()) {<br>
for (auto *M : Record->methods()) {<br>
// See if a method overloads virtual methods in a base<br>
// class without overriding any.<br>
if (!M->isStatic())<br>
DiagnoseHiddenVirtualMethods(M);<br>
-<br>
+ if (M->hasAttr<OverrideAttr>())<br>
+ HasMethodWithOverrideControl = true;<br>
+ else if (M->size_overridden_methods() > 0)<br>
+ HasOverridingMethodWithoutOverrideControl = true;<br>
// Check whether the explicitly-defaulted special members are valid.<br>
if (!M->isInvalidDecl() && M->isExplicitlyDefaulted())<br>
CheckExplicitlyDefaultedSpecialMember(M);<br>
@@ -4788,6 +4809,13 @@ void Sema::CheckCompletedCXXClass(CXXRec<br>
}<br>
}<br>
<br>
+ if (HasMethodWithOverrideControl &&<br>
+ HasOverridingMethodWithoutOverrideControl) {<br>
+ // At least one method has the 'override' control declared.<br>
+ // Diagnose all other overridden methods which do not have 'override' specified on them.<br>
+ for (auto *M : Record->methods())<br>
+ DiagnoseAbsenceOfOverrideControl(M);<br>
+ }<br>
// C++11 [dcl.constexpr]p8: A constexpr specifier for a non-static member<br>
// function that is not a constructor declares that member function to be<br>
// const. [...] The class of which that function is a member shall be<br>
<br>
Modified: cfe/trunk/test/Parser/MicrosoftExtensions.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/MicrosoftExtensions.cpp?rev=220703&r1=220702&r2=220703&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/MicrosoftExtensions.cpp?rev=220703&r1=220702&r2=220703&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/Parser/MicrosoftExtensions.cpp (original)<br>
+++ cfe/trunk/test/Parser/MicrosoftExtensions.cpp Mon Oct 27 14:11:51 2014<br>
@@ -208,12 +208,12 @@ extern TypenameWrongPlace<AAAA> PR16925;<br>
<br>
__interface MicrosoftInterface;<br>
__interface MicrosoftInterface {<br>
- void foo1() = 0;<br>
+ void foo1() = 0; // expected-note {{overridden virtual function is here}}<br>
virtual void foo2() = 0;<br>
};<br>
<br>
__interface MicrosoftDerivedInterface : public MicrosoftInterface {<br>
- void foo1();<br>
+ void foo1(); // expected-warning {{'foo1' overrides a member function but is not marked 'override'}}<br>
void foo2() override;<br>
void foo3();<br>
};<br>
<br>
Modified: cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp?rev=220703&r1=220702&r2=220703&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp?rev=220703&r1=220702&r2=220703&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp (original)<br>
+++ cfe/trunk/test/SemaCXX/MicrosoftExtensions.cpp Mon Oct 27 14:11:51 2014<br>
@@ -372,14 +372,15 @@ struct SomeBase {<br>
<br>
// expected-note@+2 {{overridden virtual function is here}}<br>
// expected-warning@+1 {{'sealed' keyword is a Microsoft extension}}<br>
- virtual void SealedFunction() sealed;<br>
+ virtual void SealedFunction() sealed; // expected-note {{overridden virtual function is here}}<br>
};<br>
<br>
// expected-note@+2 {{'SealedType' declared here}}<br>
// expected-warning@+1 {{'sealed' keyword is a Microsoft extension}}<br>
struct SealedType sealed : SomeBase {<br>
- // expected-error@+1 {{declaration of 'SealedFunction' overrides a 'sealed' function}}<br>
- virtual void SealedFunction();<br>
+ // expected-error@+2 {{declaration of 'SealedFunction' overrides a 'sealed' function}}<br>
+ // FIXME. warning can be suppressed if we're also issuing error for overriding a 'final' function.<br>
+ virtual void SealedFunction(); // expected-warning {{'SealedFunction' overrides a member function but is not marked 'override'}}<br>
<br>
// expected-warning@+1 {{'override' keyword is a C++11 extension}}<br>
virtual void OverrideMe() override;<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
</blockquote></div><br></div>