From 704c8f76bbe2de68375f7f146e75bd74de6dd518 Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Fri, 20 Apr 2012 18:46:14 +0000 Subject: [PATCH] Fix bug where a class's (deleted) copy constructor would be implicitly given a non-const reference parameter type if the class had any subobjects with deleted copy constructors. This causes a rejects-valid if the class's copy constructor is explicitly defaulted (as happens for some implementations of std::pair etc). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@155218 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/Sema/Sema.h | 29 ++++------- lib/Sema/SemaDeclCXX.cpp | 27 ++++++----- lib/Sema/SemaLookup.cpp | 45 ++++++----------- test/CXX/special/class.copy/implicit-move.cpp | 8 ++-- test/CXX/special/class.copy/p8-cxx11.cpp | 48 +++++++++++++++++++ 5 files changed, 91 insertions(+), 66 deletions(-) create mode 100644 test/CXX/special/class.copy/p8-cxx11.cpp diff --git a/include/clang/Sema/Sema.h b/include/clang/Sema/Sema.h index e84a554505..fc28ac3a53 100644 --- a/include/clang/Sema/Sema.h +++ b/include/clang/Sema/Sema.h @@ -665,23 +665,19 @@ public: /// SpecialMemberOverloadResult - The overloading result for a special member /// function. /// - /// This is basically a wrapper around PointerIntPair. The lowest bit of the - /// integer is used to determine whether we have a parameter qualification - /// match, the second-lowest is whether we had success in resolving the - /// overload to a unique non-deleted function. - /// - /// The ConstParamMatch bit represents whether, when looking up a copy - /// constructor or assignment operator, we found a potential copy - /// constructor/assignment operator whose first parameter is const-qualified. - /// This is used for determining parameter types of other objects and is - /// utterly meaningless on other types of special members. + /// This is basically a wrapper around PointerIntPair. The lowest bits of the + /// integer are used to determine whether overload resolution succeeded, and + /// whether, when looking up a copy constructor or assignment operator, we + /// found a potential copy constructor/assignment operator whose first + /// parameter is const-qualified. This is used for determining parameter types + /// of other objects and is utterly meaningless on other types of special + /// members. class SpecialMemberOverloadResult : public llvm::FastFoldingSetNode { public: enum Kind { NoMemberOrDeleted, Ambiguous, - SuccessNonConst, - SuccessConst + Success }; private: @@ -697,9 +693,6 @@ public: Kind getKind() const { return static_cast(Pair.getInt()); } void setKind(Kind K) { Pair.setInt(K); } - - bool hasSuccess() const { return getKind() >= SuccessNonConst; } - bool hasConstParamMatch() const { return getKind() == SuccessConst; } }; /// \brief A cache of special member function overload resolution results @@ -1921,11 +1914,9 @@ public: DeclContextLookupResult LookupConstructors(CXXRecordDecl *Class); CXXConstructorDecl *LookupDefaultConstructor(CXXRecordDecl *Class); CXXConstructorDecl *LookupCopyingConstructor(CXXRecordDecl *Class, - unsigned Quals, - bool *ConstParam = 0); + unsigned Quals); CXXMethodDecl *LookupCopyingAssignment(CXXRecordDecl *Class, unsigned Quals, - bool RValueThis, unsigned ThisQuals, - bool *ConstParam = 0); + bool RValueThis, unsigned ThisQuals); CXXConstructorDecl *LookupMovingConstructor(CXXRecordDecl *Class); CXXMethodDecl *LookupMovingAssignment(CXXRecordDecl *Class, bool RValueThis, unsigned ThisQuals); diff --git a/lib/Sema/SemaDeclCXX.cpp b/lib/Sema/SemaDeclCXX.cpp index f0f52c7546..1372f77136 100644 --- a/lib/Sema/SemaDeclCXX.cpp +++ b/lib/Sema/SemaDeclCXX.cpp @@ -7577,8 +7577,9 @@ Sema::ComputeDefaultedCopyAssignmentExceptionSpecAndConst( assert(!Base->getType()->isDependentType() && "Cannot generate implicit members for class with dependent bases."); CXXRecordDecl *BaseClassDecl = Base->getType()->getAsCXXRecordDecl(); - LookupCopyingAssignment(BaseClassDecl, Qualifiers::Const, false, 0, - &HasConstCopyAssignment); + HasConstCopyAssignment &= + (bool)LookupCopyingAssignment(BaseClassDecl, Qualifiers::Const, + false, 0); } // In C++11, the above citation has "or virtual" added @@ -7589,8 +7590,9 @@ Sema::ComputeDefaultedCopyAssignmentExceptionSpecAndConst( assert(!Base->getType()->isDependentType() && "Cannot generate implicit members for class with dependent bases."); CXXRecordDecl *BaseClassDecl = Base->getType()->getAsCXXRecordDecl(); - LookupCopyingAssignment(BaseClassDecl, Qualifiers::Const, false, 0, - &HasConstCopyAssignment); + HasConstCopyAssignment &= + (bool)LookupCopyingAssignment(BaseClassDecl, Qualifiers::Const, + false, 0); } } @@ -7604,8 +7606,9 @@ Sema::ComputeDefaultedCopyAssignmentExceptionSpecAndConst( ++Field) { QualType FieldType = Context.getBaseElementType((*Field)->getType()); if (CXXRecordDecl *FieldClassDecl = FieldType->getAsCXXRecordDecl()) { - LookupCopyingAssignment(FieldClassDecl, Qualifiers::Const, false, 0, - &HasConstCopyAssignment); + HasConstCopyAssignment &= + (bool)LookupCopyingAssignment(FieldClassDecl, Qualifiers::Const, + false, 0); } } @@ -8608,8 +8611,8 @@ Sema::ComputeDefaultedCopyCtorExceptionSpecAndConst(CXXRecordDecl *ClassDecl) { CXXRecordDecl *BaseClassDecl = cast(Base->getType()->getAs()->getDecl()); - LookupCopyingConstructor(BaseClassDecl, Qualifiers::Const, - &HasConstCopyConstructor); + HasConstCopyConstructor &= + (bool)LookupCopyingConstructor(BaseClassDecl, Qualifiers::Const); } for (CXXRecordDecl::base_class_iterator Base = ClassDecl->vbases_begin(), @@ -8618,8 +8621,8 @@ Sema::ComputeDefaultedCopyCtorExceptionSpecAndConst(CXXRecordDecl *ClassDecl) { ++Base) { CXXRecordDecl *BaseClassDecl = cast(Base->getType()->getAs()->getDecl()); - LookupCopyingConstructor(BaseClassDecl, Qualifiers::Const, - &HasConstCopyConstructor); + HasConstCopyConstructor &= + (bool)LookupCopyingConstructor(BaseClassDecl, Qualifiers::Const); } // -- for all the nonstatic data members of X that are of a @@ -8632,8 +8635,8 @@ Sema::ComputeDefaultedCopyCtorExceptionSpecAndConst(CXXRecordDecl *ClassDecl) { ++Field) { QualType FieldType = Context.getBaseElementType((*Field)->getType()); if (CXXRecordDecl *FieldClassDecl = FieldType->getAsCXXRecordDecl()) { - LookupCopyingConstructor(FieldClassDecl, Qualifiers::Const, - &HasConstCopyConstructor); + HasConstCopyConstructor &= + (bool)LookupCopyingConstructor(FieldClassDecl, Qualifiers::Const); } } // Otherwise, the implicitly declared copy constructor will have diff --git a/lib/Sema/SemaLookup.cpp b/lib/Sema/SemaLookup.cpp index f003bddc05..9f5138ba4a 100644 --- a/lib/Sema/SemaLookup.cpp +++ b/lib/Sema/SemaLookup.cpp @@ -2277,7 +2277,7 @@ Sema::SpecialMemberOverloadResult *Sema::LookupSpecialMember(CXXRecordDecl *RD, Result->setMethod(DD); Result->setKind(DD->isDeleted() ? SpecialMemberOverloadResult::NoMemberOrDeleted : - SpecialMemberOverloadResult::SuccessNonConst); + SpecialMemberOverloadResult::Success); return Result; } @@ -2288,6 +2288,9 @@ Sema::SpecialMemberOverloadResult *Sema::LookupSpecialMember(CXXRecordDecl *RD, Expr *Arg = 0; unsigned NumArgs; + QualType ArgType = CanTy; + ExprValueKind VK = VK_LValue; + if (SM == CXXDefaultConstructor) { Name = Context.DeclarationNames.getCXXConstructorName(CanTy); NumArgs = 0; @@ -2308,7 +2311,6 @@ Sema::SpecialMemberOverloadResult *Sema::LookupSpecialMember(CXXRecordDecl *RD, DeclareImplicitMoveAssignment(RD); } - QualType ArgType = CanTy; if (ConstArg) ArgType.addConst(); if (VolatileArg) @@ -2321,14 +2323,17 @@ Sema::SpecialMemberOverloadResult *Sema::LookupSpecialMember(CXXRecordDecl *RD, // Possibly an XValue is actually correct in the case of move, but // there is no semantic difference for class types in this restricted // case. - ExprValueKind VK; if (SM == CXXCopyConstructor || SM == CXXCopyAssignment) VK = VK_LValue; else VK = VK_RValue; + } + OpaqueValueExpr FakeArg(SourceLocation(), ArgType, VK); + + if (SM != CXXDefaultConstructor) { NumArgs = 1; - Arg = new (Context) OpaqueValueExpr(SourceLocation(), ArgType, VK); + Arg = &FakeArg; } // Create the object argument @@ -2338,17 +2343,14 @@ Sema::SpecialMemberOverloadResult *Sema::LookupSpecialMember(CXXRecordDecl *RD, if (VolatileThis) ThisTy.addVolatile(); Expr::Classification Classification = - (new (Context) OpaqueValueExpr(SourceLocation(), ThisTy, - RValueThis ? VK_RValue : VK_LValue))-> - Classify(Context); + OpaqueValueExpr(SourceLocation(), ThisTy, + RValueThis ? VK_RValue : VK_LValue).Classify(Context); // Now we perform lookup on the name we computed earlier and do overload // resolution. Lookup is only performed directly into the class since there // will always be a (possibly implicit) declaration to shadow any others. OverloadCandidateSet OCS((SourceLocation())); DeclContext::lookup_iterator I, E; - SpecialMemberOverloadResult::Kind SuccessKind = - SpecialMemberOverloadResult::SuccessNonConst; llvm::tie(I, E) = RD->lookup(Name); assert((I != E) && @@ -2378,17 +2380,6 @@ Sema::SpecialMemberOverloadResult *Sema::LookupSpecialMember(CXXRecordDecl *RD, else AddOverloadCandidate(M, DeclAccessPair::make(M, AS_public), llvm::makeArrayRef(&Arg, NumArgs), OCS, true); - - // Here we're looking for a const parameter to speed up creation of - // implicit copy methods. - if ((SM == CXXCopyAssignment && M->isCopyAssignmentOperator()) || - (SM == CXXCopyConstructor && - cast(M)->isCopyConstructor())) { - QualType ArgType = M->getType()->getAs()->getArgType(0); - if (!ArgType->isReferenceType() || - ArgType->getPointeeType().isConstQualified()) - SuccessKind = SpecialMemberOverloadResult::SuccessConst; - } } else if (FunctionTemplateDecl *Tmpl = dyn_cast(Cand)) { if (SM == CXXCopyAssignment || SM == CXXMoveAssignment) @@ -2409,7 +2400,7 @@ Sema::SpecialMemberOverloadResult *Sema::LookupSpecialMember(CXXRecordDecl *RD, switch (OCS.BestViableFunction(*this, SourceLocation(), Best)) { case OR_Success: Result->setMethod(cast(Best->Function)); - Result->setKind(SuccessKind); + Result->setKind(SpecialMemberOverloadResult::Success); break; case OR_Deleted: @@ -2442,17 +2433,13 @@ CXXConstructorDecl *Sema::LookupDefaultConstructor(CXXRecordDecl *Class) { /// \brief Look up the copying constructor for the given class. CXXConstructorDecl *Sema::LookupCopyingConstructor(CXXRecordDecl *Class, - unsigned Quals, - bool *ConstParamMatch) { + unsigned Quals) { assert(!(Quals & ~(Qualifiers::Const | Qualifiers::Volatile)) && "non-const, non-volatile qualifiers for copy ctor arg"); SpecialMemberOverloadResult *Result = LookupSpecialMember(Class, CXXCopyConstructor, Quals & Qualifiers::Const, Quals & Qualifiers::Volatile, false, false, false); - if (ConstParamMatch) - *ConstParamMatch = Result->hasConstParamMatch(); - return cast_or_null(Result->getMethod()); } @@ -2485,8 +2472,7 @@ DeclContext::lookup_result Sema::LookupConstructors(CXXRecordDecl *Class) { /// \brief Look up the copying assignment operator for the given class. CXXMethodDecl *Sema::LookupCopyingAssignment(CXXRecordDecl *Class, unsigned Quals, bool RValueThis, - unsigned ThisQuals, - bool *ConstParamMatch) { + unsigned ThisQuals) { assert(!(Quals & ~(Qualifiers::Const | Qualifiers::Volatile)) && "non-const, non-volatile qualifiers for copy assignment arg"); assert(!(ThisQuals & ~(Qualifiers::Const | Qualifiers::Volatile)) && @@ -2497,9 +2483,6 @@ CXXMethodDecl *Sema::LookupCopyingAssignment(CXXRecordDecl *Class, ThisQuals & Qualifiers::Const, ThisQuals & Qualifiers::Volatile); - if (ConstParamMatch) - *ConstParamMatch = Result->hasConstParamMatch(); - return Result->getMethod(); } diff --git a/test/CXX/special/class.copy/implicit-move.cpp b/test/CXX/special/class.copy/implicit-move.cpp index b1b298e893..3e9accfbf6 100644 --- a/test/CXX/special/class.copy/implicit-move.cpp +++ b/test/CXX/special/class.copy/implicit-move.cpp @@ -198,15 +198,15 @@ namespace DR1402 { struct NoMove4 : NonTrivialCopyAssign {}; // expected-note 2{{'const DR1402::NoMove4 &'}} struct NoMove5 : virtual NonTrivialCopyCtor {}; // expected-note 2{{'const DR1402::NoMove5 &'}} struct NoMove6 : virtual NonTrivialCopyAssign {}; // expected-note 2{{'const DR1402::NoMove6 &'}} - struct NoMove7 : NonTrivialCopyCtorVBase {}; // expected-note 2{{'DR1402::NoMove7 &'}} - struct NoMove8 : NonTrivialCopyAssignVBase {}; // expected-note 2{{'DR1402::NoMove8 &'}} + struct NoMove7 : NonTrivialCopyCtorVBase {}; // expected-note 2{{'const DR1402::NoMove7 &'}} + struct NoMove8 : NonTrivialCopyAssignVBase {}; // expected-note 2{{'const DR1402::NoMove8 &'}} // A non-trivially-move-assignable virtual base class inhibits the declaration // of a move assignment (which might move-assign the base class multiple // times). struct NoMove9 : NonTrivialMoveAssign {}; - struct NoMove10 : virtual NonTrivialMoveAssign {}; // expected-note {{'DR1402::NoMove10 &'}} - struct NoMove11 : NonTrivialMoveAssignVBase {}; // expected-note {{'DR1402::NoMove11 &'}} + struct NoMove10 : virtual NonTrivialMoveAssign {}; // expected-note {{'const DR1402::NoMove10 &'}} + struct NoMove11 : NonTrivialMoveAssignVBase {}; // expected-note {{'const DR1402::NoMove11 &'}} struct Test { friend NoMove1::NoMove1(NoMove1 &&); // expected-error {{no matching function}} diff --git a/test/CXX/special/class.copy/p8-cxx11.cpp b/test/CXX/special/class.copy/p8-cxx11.cpp new file mode 100644 index 0000000000..02e6cd1c9a --- /dev/null +++ b/test/CXX/special/class.copy/p8-cxx11.cpp @@ -0,0 +1,48 @@ +// RUN: %clang_cc1 -std=c++11 %s -verify + +// C++98 [class.copy]p5 / C++11 [class.copy]p8. + +// The implicitly-declared copy constructor for a class X will have the form +// X::X(const X&) +// if [every direct subobject] has a copy constructor whose first parameter is +// of type 'const volatile[opt] T &'. Otherwise, it will have the form +// X::X(X&) + +struct ConstCopy { + ConstCopy(const ConstCopy &); +}; + +struct NonConstCopy { + NonConstCopy(NonConstCopy &); +}; + +struct DeletedConstCopy { + DeletedConstCopy(const DeletedConstCopy &) = delete; +}; + +struct DeletedNonConstCopy { + DeletedNonConstCopy(DeletedNonConstCopy &) = delete; +}; + +struct ImplicitlyDeletedConstCopy { + ImplicitlyDeletedConstCopy(ImplicitlyDeletedConstCopy &&); +}; + + +struct A : ConstCopy {}; +struct B : NonConstCopy { ConstCopy a; }; +struct C : ConstCopy { NonConstCopy a; }; +struct D : DeletedConstCopy {}; +struct E : DeletedNonConstCopy {}; +struct F { ImplicitlyDeletedConstCopy a; }; +struct G : virtual B {}; + +struct Test { + friend A::A(const A &); + friend B::B(B &); + friend C::C(C &); + friend D::D(const D &); + friend E::E(E &); + friend F::F(const F &); + friend G::G(G &); +}; -- 2.40.0