Skip to content

Commit

Permalink
Defer checking for mismatches between the deletedness of and overriding
Browse files Browse the repository at this point in the history 
function and an overridden function until we know whether the overriding
function is deleted.

We previously did these checks when we first built the declaration,
which was too soon in some cases. We now defer all these checks to the
end of the class.

Also add missing check that a consteval function cannot override a
non-consteval function and vice versa.
  • Loading branch information
@zygoloid
zygoloid committed Mar 12, 2020
1 parent 2a2d242 commit 9975dc3
Show file tree
Hide file tree
Showing 9 changed files with 176 additions and 100 deletions.
5 changes: 4 additions & 1 deletion clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Expand Up @@ -1509,9 +1509,12 @@ def err_deleted_decl_not_first : Error<

def err_deleted_override : Error<
"deleted function %0 cannot override a non-deleted function">;

def err_non_deleted_override : Error<
"non-deleted function %0 cannot override a deleted function">;
def err_consteval_override : Error<
"consteval function %0 cannot override a non-consteval function">;
def err_non_consteval_override : Error<
"non-consteval function %0 cannot override a consteval function">;

def warn_weak_vtable : Warning<
"%0 has no out-of-line virtual method definitions; its vtable will be "
Expand Down
43 changes: 2 additions & 41 deletions clang/lib/Sema/SemaDecl.cpp
View file
Expand Up @@ -8032,30 +8032,8 @@ struct FindOverriddenMethod {
return false;
}
};

enum OverrideErrorKind { OEK_All, OEK_NonDeleted, OEK_Deleted };
} // end anonymous namespace

/// Report an error regarding overriding, along with any relevant
/// overridden methods.
///
/// \param DiagID the primary error to report.
/// \param MD the overriding method.
/// \param OEK which overrides to include as notes.
static void ReportOverrides(Sema& S, unsigned DiagID, const CXXMethodDecl *MD,
OverrideErrorKind OEK = OEK_All) {
S.Diag(MD->getLocation(), DiagID) << MD->getDeclName();
for (const CXXMethodDecl *O : MD->overridden_methods()) {
// This check (& the OEK parameter) could be replaced by a predicate, but
// without lambdas that would be overkill. This is still nicer than writing
// out the diag loop 3 times.
if ((OEK == OEK_All) ||
(OEK == OEK_NonDeleted && !O->isDeleted()) ||
(OEK == OEK_Deleted && O->isDeleted()))
S.Diag(O->getLocation(), diag::note_overridden_virtual_function);
}
}

/// AddOverriddenMethods - See if a method overrides any in the base classes,
/// and if so, check that it's a valid override and remember it.
bool Sema::AddOverriddenMethods(CXXRecordDecl *DC, CXXMethodDecl *MD) {
Expand All @@ -8064,8 +8042,6 @@ bool Sema::AddOverriddenMethods(CXXRecordDecl *DC, CXXMethodDecl *MD) {
FindOverriddenMethod FOM;
FOM.Method = MD;
FOM.S = this;
bool hasDeletedOverridenMethods = false;
bool hasNonDeletedOverridenMethods = false;
bool AddedAny = false;
if (DC->lookupInBases(FOM, Paths)) {
for (auto *I : Paths.found_decls()) {
Expand All @@ -8075,21 +8051,12 @@ bool Sema::AddOverriddenMethods(CXXRecordDecl *DC, CXXMethodDecl *MD) {
!CheckOverridingFunctionAttributes(MD, OldMD) &&
!CheckOverridingFunctionExceptionSpec(MD, OldMD) &&
!CheckIfOverriddenFunctionIsMarkedFinal(MD, OldMD)) {
hasDeletedOverridenMethods |= OldMD->isDeleted();
hasNonDeletedOverridenMethods |= !OldMD->isDeleted();
AddedAny = true;
}
}
}
}

if (hasDeletedOverridenMethods && !MD->isDeleted()) {
ReportOverrides(*this, diag::err_non_deleted_override, MD, OEK_Deleted);
}
if (hasNonDeletedOverridenMethods && MD->isDeleted()) {
ReportOverrides(*this, diag::err_deleted_override, MD, OEK_NonDeleted);
}

return AddedAny;
}

Expand Down Expand Up @@ -9095,8 +9062,7 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
}

// If a function is defined as defaulted or deleted, mark it as such now.
// FIXME: Does this ever happen? ActOnStartOfFunctionDef forces the function
// definition kind to FDK_Definition.
// We'll do the relevant checks on defaulted / deleted functions later.
switch (D.getFunctionDefinitionKind()) {
case FDK_Declaration:
case FDK_Definition:
Expand Down Expand Up @@ -10745,12 +10711,7 @@ bool Sema::CheckFunctionDeclaration(Scope *S, FunctionDecl *NewFD,
if (!Method->isFunctionTemplateSpecialization() &&
!Method->getDescribedFunctionTemplate() &&
Method->isCanonicalDecl()) {
if (AddOverriddenMethods(Method->getParent(), Method)) {
// If the function was marked as "static", we have a problem.
if (NewFD->getStorageClass() == SC_Static) {
ReportOverrides(*this, diag::err_static_overrides_virtual, Method);
}
}
AddOverriddenMethods(Method->getParent(), Method);
}
if (Method->isVirtual() && NewFD->getTrailingRequiresClause())
// C++2a [class.virtual]p6
Expand Down
139 changes: 98 additions & 41 deletions clang/lib/Sema/SemaDeclCXX.cpp
View file
Expand Up @@ -6172,27 +6172,31 @@ Sema::getDefaultedFunctionKind(const FunctionDecl *FD) {
return DefaultedFunctionKind();
}

static void DefineImplicitSpecialMember(Sema &S, CXXMethodDecl *MD,
SourceLocation DefaultLoc) {
switch (S.getSpecialMember(MD)) {
static void DefineDefaultedFunction(Sema &S, FunctionDecl *FD,
SourceLocation DefaultLoc) {
Sema::DefaultedFunctionKind DFK = S.getDefaultedFunctionKind(FD);
if (DFK.isComparison())
return S.DefineDefaultedComparison(DefaultLoc, FD, DFK.asComparison());

switch (DFK.asSpecialMember()) {
case Sema::CXXDefaultConstructor:
S.DefineImplicitDefaultConstructor(DefaultLoc,
cast<CXXConstructorDecl>(MD));
cast<CXXConstructorDecl>(FD));
break;
case Sema::CXXCopyConstructor:
S.DefineImplicitCopyConstructor(DefaultLoc, cast<CXXConstructorDecl>(MD));
S.DefineImplicitCopyConstructor(DefaultLoc, cast<CXXConstructorDecl>(FD));
break;
case Sema::CXXCopyAssignment:
S.DefineImplicitCopyAssignment(DefaultLoc, MD);
S.DefineImplicitCopyAssignment(DefaultLoc, cast<CXXMethodDecl>(FD));
break;
case Sema::CXXDestructor:
S.DefineImplicitDestructor(DefaultLoc, cast<CXXDestructorDecl>(MD));
S.DefineImplicitDestructor(DefaultLoc, cast<CXXDestructorDecl>(FD));
break;
case Sema::CXXMoveConstructor:
S.DefineImplicitMoveConstructor(DefaultLoc, cast<CXXConstructorDecl>(MD));
S.DefineImplicitMoveConstructor(DefaultLoc, cast<CXXConstructorDecl>(FD));
break;
case Sema::CXXMoveAssignment:
S.DefineImplicitMoveAssignment(DefaultLoc, MD);
S.DefineImplicitMoveAssignment(DefaultLoc, cast<CXXMethodDecl>(FD));
break;
case Sema::CXXInvalid:
llvm_unreachable("Invalid special member.");
Expand Down Expand Up @@ -6313,6 +6317,28 @@ static bool canPassInRegisters(Sema &S, CXXRecordDecl *D,
return HasNonDeletedCopyOrMove;
}

/// Report an error regarding overriding, along with any relevant
/// overridden methods.
///
/// \param DiagID the primary error to report.
/// \param MD the overriding method.
/// \param OEK which overrides to include as notes.
static bool
ReportOverrides(Sema &S, unsigned DiagID, const CXXMethodDecl *MD,
llvm::function_ref<bool(const CXXMethodDecl *)> Report) {
bool IssuedDiagnostic = false;
for (const CXXMethodDecl *O : MD->overridden_methods()) {
if (Report(O)) {
if (!IssuedDiagnostic) {
S.Diag(MD->getLocation(), DiagID) << MD->getDeclName();
IssuedDiagnostic = true;
}
S.Diag(O->getLocation(), diag::note_overridden_virtual_function);
}
}
return IssuedDiagnostic;
}

/// Perform semantic checks on a class definition that has been
/// completing, introducing implicitly-declared members, checking for
/// abstract types, etc.
Expand Down Expand Up @@ -6427,21 +6453,64 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
// primary comparison functions (==, <=>).
llvm::SmallVector<FunctionDecl*, 5> DefaultedSecondaryComparisons;

auto CheckForDefaultedFunction = [&](FunctionDecl *FD) {
if (!FD || FD->isInvalidDecl() || !FD->isExplicitlyDefaulted())
// Perform checks that can't be done until we know all the properties of a
// member function (whether it's defaulted, deleted, virtual, overriding,
// ...).
auto CheckCompletedMemberFunction = [&](CXXMethodDecl *MD) {
// A static function cannot override anything.
if (MD->getStorageClass() == SC_Static) {
if (ReportOverrides(*this, diag::err_static_overrides_virtual, MD,
[](const CXXMethodDecl *) { return true; }))
return;
}

// A deleted function cannot override a non-deleted function and vice
// versa.
if (ReportOverrides(*this,
MD->isDeleted() ? diag::err_deleted_override
: diag::err_non_deleted_override,
MD, [&](const CXXMethodDecl *V) {
return MD->isDeleted() != V->isDeleted();
})) {
if (MD->isDefaulted() && MD->isDeleted())
// Explain why this defaulted function was deleted.
DiagnoseDeletedDefaultedFunction(MD);
return;
}

// A consteval function cannot override a non-consteval function and vice
// versa.
if (ReportOverrides(*this,
MD->isConsteval() ? diag::err_consteval_override
: diag::err_non_consteval_override,
MD, [&](const CXXMethodDecl *V) {
return MD->isConsteval() != V->isConsteval();
})) {
if (MD->isDefaulted() && MD->isDeleted())
// Explain why this defaulted function was deleted.
DiagnoseDeletedDefaultedFunction(MD);
return;
}
};

auto CheckForDefaultedFunction = [&](FunctionDecl *FD) -> bool {
if (!FD || FD->isInvalidDecl() || !FD->isExplicitlyDefaulted())
return false;

DefaultedFunctionKind DFK = getDefaultedFunctionKind(FD);
if (DFK.asComparison() == DefaultedComparisonKind::NotEqual ||
DFK.asComparison() == DefaultedComparisonKind::Relational)
DFK.asComparison() == DefaultedComparisonKind::Relational) {
DefaultedSecondaryComparisons.push_back(FD);
else
CheckExplicitlyDefaultedFunction(S, FD);
return true;
}

CheckExplicitlyDefaultedFunction(S, FD);
return false;
};

auto CompleteMemberFunction = [&](CXXMethodDecl *M) {
// Check whether the explicitly-defaulted members are valid.
CheckForDefaultedFunction(M);
bool Incomplete = CheckForDefaultedFunction(M);

// Skip the rest of the checks for a member of a dependent class.
if (Record->isDependentType())
Expand Down Expand Up @@ -6488,7 +6557,10 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
// function right away.
// FIXME: We can defer doing this until the vtable is marked as used.
if (M->isDefaulted() && M->isConstexpr() && M->size_overridden_methods())
DefineImplicitSpecialMember(*this, M, M->getLocation());
DefineDefaultedFunction(*this, M, M->getLocation());

if (!Incomplete)
CheckCompletedMemberFunction(M);
};

// Check the destructor before any other member function. We need to
Expand Down Expand Up @@ -6534,9 +6606,14 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
}

// Check the defaulted secondary comparisons after any other member functions.
for (FunctionDecl *FD : DefaultedSecondaryComparisons)
for (FunctionDecl *FD : DefaultedSecondaryComparisons) {
CheckExplicitlyDefaultedFunction(S, FD);

// If this is a member function, we deferred checking it until now.
if (auto *MD = dyn_cast<CXXMethodDecl>(FD))
CheckCompletedMemberFunction(MD);
}

// ms_struct is a request to use the same ABI rules as MSVC. Check
// whether this class uses any C++ features that are implemented
// completely differently in MSVC, and if so, emit a diagnostic.
Expand Down Expand Up @@ -13023,7 +13100,7 @@ void Sema::ActOnFinishCXXNonNestedClass() {
SmallVector<CXXMethodDecl*, 4> WorkList;
std::swap(DelayedDllExportMemberFunctions, WorkList);
for (CXXMethodDecl *M : WorkList) {
DefineImplicitSpecialMember(*this, M, M->getLocation());
DefineDefaultedFunction(*this, M, M->getLocation());

// Pass the method to the consumer to get emitted. This is not necessary
// for explicit instantiation definitions, as they will get emitted
Expand Down Expand Up @@ -16368,9 +16445,6 @@ void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc) {
Fn->setInvalidDecl();
}

if (Fn->isDeleted())
return;

// C++11 [basic.start.main]p3:
// A program that defines main as deleted [...] is ill-formed.
if (Fn->isMain())
Expand All @@ -16380,25 +16454,6 @@ void Sema::SetDeclDeleted(Decl *Dcl, SourceLocation DelLoc) {
// A deleted function is implicitly inline.
Fn->setImplicitlyInline();
Fn->setDeletedAsWritten();

// See if we're deleting a function which is already known to override a
// non-deleted virtual function.
if (CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(Fn)) {
bool IssuedDiagnostic = false;
for (const CXXMethodDecl *O : MD->overridden_methods()) {
if (!(*MD->begin_overridden_methods())->isDeleted()) {
if (!IssuedDiagnostic) {
Diag(DelLoc, diag::err_deleted_override) << MD->getDeclName();
IssuedDiagnostic = true;
}
Diag(O->getLocation(), diag::note_overridden_virtual_function);
}
}
// If this function was implicitly deleted because it was defaulted,
// explain why it was deleted.
if (IssuedDiagnostic && MD->isDefaulted())
DiagnoseDeletedDefaultedFunction(MD);
}
}

void Sema::SetDeclDefaulted(Decl *Dcl, SourceLocation DefaultLoc) {
Expand Down Expand Up @@ -16480,10 +16535,12 @@ void Sema::SetDeclDefaulted(Decl *Dcl, SourceLocation DefaultLoc) {
if (Primary->getCanonicalDecl()->isDefaulted())
return;

// FIXME: Once we support defining comparisons out of class, check for a
// defaulted comparison here.
if (CheckExplicitlyDefaultedSpecialMember(MD, DefKind.asSpecialMember()))
MD->setInvalidDecl();
else
DefineImplicitSpecialMember(*this, MD, DefaultLoc);
DefineDefaultedFunction(*this, MD, DefaultLoc);
}

static void SearchForReturnInStmt(Sema &Self, Stmt *S) {
Expand Down
6 changes: 2 additions & 4 deletions clang/test/CXX/class.derived/class.abstract/p16.cpp
View file
Expand Up @@ -35,15 +35,13 @@ struct E : D {};
// expected-error@-1 {{deleted function '~E' cannot override a non-deleted function}}
// expected-note@-2 {{destructor of 'E' is implicitly deleted because base class 'D' has an inaccessible destructor}}
// expected-error@-3 {{deleted function 'operator=' cannot override a non-deleted function}}
// expected-note@-4 {{while declaring the implicit copy assignment operator for 'E'}}
// expected-note@-5 {{copy assignment operator of 'E' is implicitly deleted because base class 'D' has an inaccessible copy assignment operator}}
// expected-note@-4 {{copy assignment operator of 'E' is implicitly deleted because base class 'D' has an inaccessible copy assignment operator}}
struct F : D {};
struct G : D {};
// expected-error@-1 {{deleted function '~G' cannot override a non-deleted function}}
// expected-note@-2 {{destructor of 'G' is implicitly deleted because base class 'D' has an inaccessible destructor}}
// expected-error@-3 {{deleted function 'operator=' cannot override a non-deleted function}}
// expected-note@-4 {{while declaring the implicit move assignment operator for 'G'}}
// expected-note@-5 {{move assignment operator of 'G' is implicitly deleted because base class 'D' has an inaccessible move assignment operator}}
// expected-note@-4 {{move assignment operator of 'G' is implicitly deleted because base class 'D' has an inaccessible move assignment operator}}
struct H : D { // expected-note {{deleted because base class 'D' has an inaccessible move assignment}}
H &operator=(H&&) = default; // expected-warning {{implicitly deleted}}
// expected-error@-1 {{deleted function 'operator=' cannot override a non-deleted function}}
Expand Down
11 changes: 9 additions & 2 deletions clang/test/CXX/special/class.dtor/p5-0x.cpp
View file
Expand Up @@ -86,9 +86,9 @@ struct C4 : virtual InaccessibleDtor { C4(); } c4; // expected-error {{deleted f
// -- for a virtual destructor, lookup of the non-array deallocation function
// results in an ambiguity or a function that is deleted or inaccessible.
class D1 {
void operator delete(void*);
void operator delete(void*); // expected-note {{here}}
public:
virtual ~D1() = default; // expected-note {{here}}
virtual ~D1() = default; // expected-note 2{{here}}
} d1; // ok
struct D2 : D1 { // expected-note 2{{virtual destructor requires an unambiguous, accessible 'operator delete'}} \
// expected-error {{deleted function '~D2' cannot override a non-deleted}}
Expand All @@ -103,3 +103,10 @@ struct D4 { // expected-note {{virtual destructor requires an unambiguous, acces
virtual ~D4() = default; // expected-note {{implicitly deleted here}}
void operator delete(void*) = delete;
} d4; // expected-error {{deleted function}}
struct D5 : D1 {
~D5(); // ok (but not definable)
} d5;
D5::~D5() {} // expected-error {{'operator delete' is a private member of 'D1'}}
struct D6 : D1 {
~D6() = delete; // expected-error {{deleted function '~D6' cannot override a non-deleted}}
};
7 changes: 1 addition & 6 deletions clang/test/SemaCXX/PR9572.cpp
View file
Expand Up @@ -16,8 +16,7 @@ struct Foo : public Base {
// expected-error@-2 {{base class 'Base' has private destructor}}
#else
// expected-error@-4 {{deleted function '~Foo' cannot override a non-deleted function}}
// expected-note@-5 {{overridden virtual function is here}}
// expected-note@-6 3 {{destructor of 'Foo' is implicitly deleted because base class 'Base' has an inaccessible destructor}}
// expected-note@-5 3{{destructor of 'Foo' is implicitly deleted because base class 'Base' has an inaccessible destructor}}
#endif

const int kBlah = 3;
Expand All @@ -29,10 +28,6 @@ struct Foo : public Base {
};

struct Bar : public Foo {
#if __cplusplus >= 201103L
// expected-error@-2 {{non-deleted function '~Bar' cannot override a deleted function}}
// expected-note@-3 {{while declaring the implicit destructor for 'Bar'}}
#endif
Bar() { }
#if __cplusplus <= 199711L
// expected-note@-2 {{implicit destructor for 'Foo' first required here}}
Expand Down
6 changes: 6 additions & 0 deletions clang/test/SemaCXX/cxx0x-cursory-default-delete.cpp
View file
Expand Up @@ -211,3 +211,9 @@ struct DeletedMemberTemplateTooLate {
};
template<typename T> void DeletedMemberTemplateTooLate::f() = delete; // expected-error {{must be first decl}} expected-note {{substitution failure}}
void use_member_template_deleted_too_late() { DeletedMemberTemplateTooLate::f<int>(); } // expected-error {{no matching function}}

namespace deleted_overrides_deleted {
struct A { virtual void f() = delete; };
template<typename T> struct B : A { virtual void f() = delete; };
template struct B<int>;
}

0 comments on commit 9975dc3

Please sign in to comment.