| Here is a new patch for warning on missing ‘override’ with following changes from the old patch. 1. I have incorporated few refactoring suggested by you and others. 2. I removed ‘FixIt’ for now. It is not necessary for this patch and something we can incorporate later if this is something we want to do. 3. After reading all the discussions I no longer warn on missing ‘override’ on ‘final’ methods. I see the point made that it is redundant for ‘final’ which already means ‘override. And since it is in violation of coding guidelines, it is more likely to break code. I also understand that it is a contentious issue which may be changed later. 4. This patch warns if overriding method (without ‘override' keyword) is explicitly declared as ‘virtual’. I feel that this is a useful warning as ‘override’ keyword makes user’s intention verbose. But, I can be convinced otherwise. - Please review. - Fariborz |
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td (revision 219503)
+++ include/clang/Basic/DiagnosticGroups.td (working copy)
@@ -146,6 +146,8 @@
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]>;
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 219503)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -1689,6 +1689,9 @@
"%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<
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h (revision 219503)
+++ include/clang/Sema/Sema.h (working copy)
@@ -5084,6 +5084,10 @@
/// 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
Index: lib/Sema/SemaDeclCXX.cpp
===================================================================
--- lib/Sema/SemaDeclCXX.cpp (revision 219503)
+++ lib/Sema/SemaDeclCXX.cpp (working copy)
@@ -1897,6 +1897,22 @@
<< MD->getDeclName();
}
+void Sema::DiagnoseAbsenceOfOverrideControl(NamedDecl *D) {
+ if (D->isInvalidDecl())
+ 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.
@@ -4680,13 +4696,18 @@
}
}
+ 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);
@@ -4705,6 +4726,14 @@
}
}
+ 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>())
+ 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
Index: test/Parser/MicrosoftExtensions.cpp
===================================================================
--- test/Parser/MicrosoftExtensions.cpp (revision 219503)
+++ test/Parser/MicrosoftExtensions.cpp (working copy)
@@ -208,12 +208,12 @@
__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();
};
Index: test/SemaCXX/MicrosoftExtensions.cpp
===================================================================
--- test/SemaCXX/MicrosoftExtensions.cpp (revision 219503)
+++ test/SemaCXX/MicrosoftExtensions.cpp (working copy)
@@ -372,14 +372,14 @@
// 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;
On Oct 2, 2014, at 5:24 PM, Richard Smith <[email protected]> wrote:
|
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
