Add a warning for overridden methods that are missing the 'override' keyword.
This diagnostic is only triggered when at least one method is marked 'override'
in the class, and only in C++11 mode.
This way, codebase owners can be more confident when adding 'override' - it
can be added one class at a time while asserting that methods are 'override'
iff they override a base class's method.
http://llvm-reviews.chandlerc.com/D1275
Files:
include/clang/AST/DeclCXX.h
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/DeclCXX.cpp
lib/Sema/SemaDeclCXX.cpp
test/CXX/class.derived/class.virtual/p3-0x.cpp
test/FixIt/fixit-cxx0x.cpp
test/Misc/diag-override.cpp
test/Parser/MicrosoftExtensions.cpp
test/SemaCXX/cxx98-compat.cpp
test/SemaCXX/ms-interface.cpp
Index: include/clang/AST/DeclCXX.h
===================================================================
--- include/clang/AST/DeclCXX.h
+++ include/clang/AST/DeclCXX.h
@@ -455,6 +455,9 @@
/// \brief Whether this class describes a C++ lambda.
bool IsLambda : 1;
+ /// \brief Whether this class has any override annotations
+ bool HasOverrideAnnotation : 1;
+
/// \brief The number of base class specifiers in Bases.
unsigned NumBases;
@@ -1220,6 +1223,11 @@
/// (C++11 [class]p6).
bool isTriviallyCopyable() const;
+ /// \brief Whether this class has any methods with an override annotation.
+ bool hasOverrideAnnotation() const {
+ return data().HasOverrideAnnotation;
+ }
+
/// \brief Determine whether this class is considered trivial.
///
/// C++11 [class]p6:
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -1535,6 +1535,9 @@
"base %0 is marked 'final'">;
def warn_abstract_final_class : Warning<
"abstract class is marked 'final'">, InGroup<AbstractFinalClass>;
+def warn_missing_override_annotation : Warning<
+ "method %0 overrides %1 method(s) but is not marked 'override'">,
+ InGroup<DiagGroup<"missing-override-annotation">>;
// C++11 attributes
def err_repeat_attribute : Error<"'%0' attribute cannot be repeated">;
Index: lib/AST/DeclCXX.cpp
===================================================================
--- lib/AST/DeclCXX.cpp
+++ lib/AST/DeclCXX.cpp
@@ -11,6 +11,7 @@
//
//===----------------------------------------------------------------------===//
+#include "clang/AST/Attr.h"
#include "clang/AST/DeclCXX.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/ASTMutationListener.h"
@@ -22,6 +23,8 @@
#include "clang/Basic/IdentifierTable.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallPtrSet.h"
+
+
using namespace clang;
//===----------------------------------------------------------------------===//
@@ -61,8 +64,8 @@
HasDeclaredCopyConstructorWithConstParam(false),
HasDeclaredCopyAssignmentWithConstParam(false),
FailedImplicitMoveConstructor(false), FailedImplicitMoveAssignment(false),
- IsLambda(false), NumBases(0), NumVBases(0), Bases(), VBases(),
- Definition(D), FirstFriend() {
+ IsLambda(false), HasOverrideAnnotation(false), NumBases(0), NumVBases(0),
+ Bases(), VBases(), Definition(D), FirstFriend() {
}
CXXBaseSpecifier *CXXRecordDecl::DefinitionData::getBasesSlowCase() const {
@@ -537,6 +540,7 @@
data().HasDeclaredCopyAssignmentWithConstParam = true;
}
+
if (Method->isMoveAssignmentOperator())
SMKind |= SMF_MoveAssignment;
@@ -1237,6 +1241,15 @@
E = data().Conversions.end();
I != E; ++I)
I.setAccess((*I)->getAccess());
+
+ // If any of this class's methods are marked `override`, note that here.
+ for (CXXRecordDecl::method_iterator I = method_begin(), E = method_end();
+ I != E; ++I) {
+ if (I->hasAttr<OverrideAttr>()) {
+ data().HasOverrideAnnotation = true;
+ break;
+ }
+ }
}
bool CXXRecordDecl::mayBeAbstract() const {
Index: lib/Sema/SemaDeclCXX.cpp
===================================================================
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -4140,6 +4140,17 @@
}
}
+// Issue a diagnostic if the method declaration overrides some other method, but
+// does not have an override specifier.
+static void CheckMissingOverrideSpecifier(Sema &S, CXXMethodDecl *MD) {
+ if (MD->isVirtual() && !MD->hasAttr<OverrideAttr>() &&
+ MD->size_overridden_methods())
+ S.Diag(MD->getLocEnd(), diag::warn_missing_override_annotation)
+ << MD << MD->size_overridden_methods()
+ << MD->getSourceRange()
+ << FixItHint::CreateInsertion(MD->getLocEnd(), " override ");
+}
+
/// \brief Perform semantic checks on a class definition that has been
/// completing, introducing implicitly-declared members, checking for
/// abstract types, etc.
@@ -4231,6 +4242,12 @@
if (!M->isInvalidDecl() && M->isExplicitlyDefaulted())
CheckExplicitlyDefaultedSpecialMember(*M);
+ // Check if any there are any missing overrides, but only if the class
+ // uses an `override` specifier somewhere.
+ // Also avoid issuing this diagnostic if not in C++11 mode.
+ if (getLangOpts().CPlusPlus11 && Record->hasOverrideAnnotation())
+ CheckMissingOverrideSpecifier(*this, *M);
+
// For an explicitly defaulted or deleted special member, we defer
// determining triviality until the class is complete. That time is now!
if (!M->isImplicit() && !M->isUserProvided()) {
Index: test/CXX/class.derived/class.virtual/p3-0x.cpp
===================================================================
--- test/CXX/class.derived/class.virtual/p3-0x.cpp
+++ test/CXX/class.derived/class.virtual/p3-0x.cpp
@@ -83,7 +83,7 @@
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 {{but is not marked 'override'}}
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 @@
template<typename T> struct Y : T {
void f() override;
- void h() final;
+ void h() final; // expected-warning {{but is not marked 'override'}}
};
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}}
}
Index: test/FixIt/fixit-cxx0x.cpp
===================================================================
--- test/FixIt/fixit-cxx0x.cpp
+++ test/FixIt/fixit-cxx0x.cpp
@@ -33,7 +33,7 @@
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 {{but is not marked 'override'}}
f3 override, // expected-error {{expected ';' at end of declaration}}
};
}
Index: test/Misc/diag-override.cpp
===================================================================
--- /dev/null
+++ test/Misc/diag-override.cpp
@@ -0,0 +1,13 @@
+// RUN: %clang_cc1 -verify -std=c++11 %s
+class Base {
+ virtual void annotated_override();
+ virtual void missing_annotation();
+};
+
+class Descendant : public Base {
+ virtual void annotated_override() override;
+ // expected-error@+1 {{does not override any member functions}}
+ virtual void not_overriding_anything() override;
+ // expected-warning@+1 {{but is not marked 'override'}}
+ virtual void missing_annotation();
+};
Index: test/Parser/MicrosoftExtensions.cpp
===================================================================
--- test/Parser/MicrosoftExtensions.cpp
+++ test/Parser/MicrosoftExtensions.cpp
@@ -179,7 +179,7 @@
};
__interface MicrosoftDerivedInterface : public MicrosoftInterface {
- void foo1();
+ void foo1(); // expected-warning {{but is not marked 'override'}}
void foo2() override;
void foo3();
};
Index: test/SemaCXX/cxx98-compat.cpp
===================================================================
--- test/SemaCXX/cxx98-compat.cpp
+++ test/SemaCXX/cxx98-compat.cpp
@@ -124,7 +124,7 @@
};
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 {{but is not marked 'override'}}
};
using AliasDecl = int; // expected-warning {{alias declarations are incompatible with C++98}}
Index: test/SemaCXX/ms-interface.cpp
===================================================================
--- test/SemaCXX/ms-interface.cpp
+++ test/SemaCXX/ms-interface.cpp
@@ -44,7 +44,7 @@
__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 {{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
