From 7c2bcc9eb038df5fa238276749a88f289e9861ae Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 7 Sep 2016 02:14:33 +0000 Subject: [PATCH] Fix clang's handling of the copy performed in the second phase of class copy-initialization. We previously got this wrong in a couple of ways: - we only looked for copy / move constructors and constructor templates for this copy, and thus would fail to copy in cases where doing so should use some other constructor (but see core issue 670), - we mishandled the special case for disabling user-defined conversions that blocks infinite recursion through repeated application of a copy constructor (applying it in slightly too many cases) -- though as far as I can tell, this does not ever actually affect the result of overload resolution, and - we misapplied the special-case rules for constructors taking a parameter whose type is a (reference to) the same class type by incorrectly assuming that only happens for copy/move constructors (it also happens for constructors instantiated from templates and those inherited from base classes). These changes should only affect strange corner cases (for instance, where the copy constructor exists but has a non-const-qualified parameter type), so for the most part it only causes us to produce more 'candidate' notes, but see the test changes for other cases whose behavior is affected. llvm-svn: 280776 --- clang/lib/Sema/SemaInit.cpp | 225 ++++++++++----------- .../dcl.init/dcl.init.ref/p5-cxx03-extra-copy.cpp | 6 +- clang/test/CXX/drs/dr0xx.cpp | 6 +- clang/test/CXX/drs/dr1xx.cpp | 2 +- clang/test/CodeGenCXX/copy-constructor-elim-2.cpp | 20 ++ clang/test/SemaCXX/conditional-expr.cpp | 2 +- clang/test/SemaCXX/copy-initialization.cpp | 11 +- .../test/SemaCXX/cxx0x-initializer-constructor.cpp | 5 +- clang/test/SemaCXX/cxx11-inheriting-ctors.cpp | 32 ++- clang/test/SemaCXX/cxx98-compat-flags.cpp | 2 +- clang/test/SemaCXX/cxx98-compat-pedantic.cpp | 2 +- 11 files changed, 180 insertions(+), 133 deletions(-) diff --git a/clang/lib/Sema/SemaInit.cpp b/clang/lib/Sema/SemaInit.cpp index 386c7ab..725afc04 100644 --- a/clang/lib/Sema/SemaInit.cpp +++ b/clang/lib/Sema/SemaInit.cpp @@ -3438,6 +3438,23 @@ static bool TryInitializerListConstruction(Sema &S, return true; } +/// Determine if the constructor has the signature of a copy or move +/// constructor for the type T of the class in which it was found. That is, +/// determine if its first parameter is of type T or reference to (possibly +/// cv-qualified) T. +static bool hasCopyOrMoveCtorParam(ASTContext &Ctx, + const ConstructorInfo &Info) { + if (Info.Constructor->getNumParams() == 0) + return false; + + QualType ParmT = + Info.Constructor->getParamDecl(0)->getType().getNonReferenceType(); + QualType ClassT = + Ctx.getRecordType(cast(Info.FoundDecl->getDeclContext())); + + return Ctx.hasSameUnqualifiedType(ParmT, ClassT); +} + static OverloadingResult ResolveConstructorOverload(Sema &S, SourceLocation DeclLoc, MultiExprArg Args, @@ -3445,59 +3462,56 @@ ResolveConstructorOverload(Sema &S, SourceLocation DeclLoc, DeclContext::lookup_result Ctors, OverloadCandidateSet::iterator &Best, bool CopyInitializing, bool AllowExplicit, - bool OnlyListConstructors, bool IsListInit) { + bool OnlyListConstructors, bool IsListInit, + bool SecondStepOfCopyInit = false) { CandidateSet.clear(); for (NamedDecl *D : Ctors) { auto Info = getConstructorInfo(D); - if (!Info.Constructor) + if (!Info.Constructor || Info.Constructor->isInvalidDecl()) continue; - bool SuppressUserConversions = false; - - if (!Info.ConstructorTmpl) { - // C++11 [over.best.ics]p4: - // ... and the constructor or user-defined conversion function is a - // candidate by - // - 13.3.1.3, when the argument is the temporary in the second step - // of a class copy-initialization, or - // - 13.3.1.4, 13.3.1.5, or 13.3.1.6 (in all cases), - // user-defined conversion sequences are not considered. - // FIXME: This breaks backward compatibility, e.g. PR12117. As a - // temporary fix, let's re-instate the third bullet above until - // there is a resolution in the standard, i.e., - // - 13.3.1.7 when the initializer list has exactly one element that is - // itself an initializer list and a conversion to some class X or - // reference to (possibly cv-qualified) X is considered for the first - // parameter of a constructor of X. - if ((CopyInitializing || - (IsListInit && Args.size() == 1 && isa(Args[0]))) && - Info.Constructor->isCopyOrMoveConstructor()) - SuppressUserConversions = true; - } + if (!AllowExplicit && Info.Constructor->isExplicit()) + continue; - if (!Info.Constructor->isInvalidDecl() && - (AllowExplicit || !Info.Constructor->isExplicit()) && - (!OnlyListConstructors || S.isInitListConstructor(Info.Constructor))) { - if (Info.ConstructorTmpl) - S.AddTemplateOverloadCandidate(Info.ConstructorTmpl, Info.FoundDecl, - /*ExplicitArgs*/ nullptr, Args, - CandidateSet, SuppressUserConversions); - else { - // C++ [over.match.copy]p1: - // - When initializing a temporary to be bound to the first parameter - // of a constructor that takes a reference to possibly cv-qualified - // T as its first argument, called with a single argument in the - // context of direct-initialization, explicit conversion functions - // are also considered. - bool AllowExplicitConv = AllowExplicit && !CopyInitializing && - Args.size() == 1 && - Info.Constructor->isCopyOrMoveConstructor(); - S.AddOverloadCandidate(Info.Constructor, Info.FoundDecl, Args, - CandidateSet, SuppressUserConversions, - /*PartialOverloading=*/false, - /*AllowExplicit=*/AllowExplicitConv); - } + if (OnlyListConstructors && !S.isInitListConstructor(Info.Constructor)) + continue; + + // C++11 [over.best.ics]p4: + // ... and the constructor or user-defined conversion function is a + // candidate by + // - 13.3.1.3, when the argument is the temporary in the second step + // of a class copy-initialization, or + // - 13.3.1.4, 13.3.1.5, or 13.3.1.6 (in all cases), [not handled here] + // - the second phase of 13.3.1.7 when the initializer list has exactly + // one element that is itself an initializer list, and the target is + // the first parameter of a constructor of class X, and the conversion + // is to X or reference to (possibly cv-qualified X), + // user-defined conversion sequences are not considered. + bool SuppressUserConversions = + SecondStepOfCopyInit || + (IsListInit && Args.size() == 1 && isa(Args[0]) && + hasCopyOrMoveCtorParam(S.Context, Info)); + + if (Info.ConstructorTmpl) + S.AddTemplateOverloadCandidate(Info.ConstructorTmpl, Info.FoundDecl, + /*ExplicitArgs*/ nullptr, Args, + CandidateSet, SuppressUserConversions); + else { + // C++ [over.match.copy]p1: + // - When initializing a temporary to be bound to the first parameter + // of a constructor [for type T] that takes a reference to possibly + // cv-qualified T as its first argument, called with a single + // argument in the context of direct-initialization, explicit + // conversion functions are also considered. + // FIXME: What if a constructor template instantiates to such a signature? + bool AllowExplicitConv = AllowExplicit && !CopyInitializing && + Args.size() == 1 && + hasCopyOrMoveCtorParam(S.Context, Info); + S.AddOverloadCandidate(Info.Constructor, Info.FoundDecl, Args, + CandidateSet, SuppressUserConversions, + /*PartialOverloading=*/false, + /*AllowExplicit=*/AllowExplicitConv); } } @@ -5348,50 +5362,6 @@ static bool shouldDestroyTemporary(const InitializedEntity &Entity) { llvm_unreachable("missed an InitializedEntity kind?"); } -/// \brief Look for copy and move constructors and constructor templates, for -/// copying an object via direct-initialization (per C++11 [dcl.init]p16). -static void LookupCopyAndMoveConstructors(Sema &S, - OverloadCandidateSet &CandidateSet, - CXXRecordDecl *Class, - Expr *CurInitExpr) { - DeclContext::lookup_result R = S.LookupConstructors(Class); - // The container holding the constructors can under certain conditions - // be changed while iterating (e.g. because of deserialization). - // To be safe we copy the lookup results to a new container. - SmallVector Ctors(R.begin(), R.end()); - for (SmallVectorImpl::iterator - CI = Ctors.begin(), CE = Ctors.end(); CI != CE; ++CI) { - NamedDecl *D = *CI; - auto Info = getConstructorInfo(D); - if (!Info.Constructor) - continue; - - if (!Info.ConstructorTmpl) { - // Handle copy/move constructors, only. - if (Info.Constructor->isInvalidDecl() || - !Info.Constructor->isCopyOrMoveConstructor() || - !Info.Constructor->isConvertingConstructor(/*AllowExplicit=*/true)) - continue; - - S.AddOverloadCandidate(Info.Constructor, Info.FoundDecl, - CurInitExpr, CandidateSet); - continue; - } - - // Handle constructor templates. - if (Info.ConstructorTmpl->isInvalidDecl()) - continue; - - if (!Info.Constructor->isConvertingConstructor(/*AllowExplicit=*/true)) - continue; - - // FIXME: Do we need to limit this to copy-constructor-like - // candidates? - S.AddTemplateOverloadCandidate(Info.ConstructorTmpl, Info.FoundDecl, - nullptr, CurInitExpr, CandidateSet, true); - } -} - /// \brief Get the location at which initialization diagnostics should appear. static SourceLocation getInitializationLoc(const InitializedEntity &Entity, Expr *Initializer) { @@ -5462,39 +5432,24 @@ static ExprResult CopyObject(Sema &S, if (!Class) return CurInit; - // C++0x [class.copy]p32: - // When certain criteria are met, an implementation is allowed to - // omit the copy/move construction of a class object, even if the - // copy/move constructor and/or destructor for the object have - // side effects. [...] - // - when a temporary class object that has not been bound to a - // reference (12.2) would be copied/moved to a class object - // with the same cv-unqualified type, the copy/move operation - // can be omitted by constructing the temporary object - // directly into the target of the omitted copy/move - // - // Note that the other three bullets are handled elsewhere. Copy - // elision for return statements and throw expressions are handled as part - // of constructor initialization, while copy elision for exception handlers - // is handled by the run-time. - bool Elidable = CurInitExpr->isTemporaryObject(S.Context, Class); SourceLocation Loc = getInitializationLoc(Entity, CurInit.get()); // Make sure that the type we are copying is complete. if (S.RequireCompleteType(Loc, T, diag::err_temp_copy_incomplete)) return CurInit; - // Perform overload resolution using the class's copy/move constructors. - // Only consider constructors and constructor templates. Per - // C++0x [dcl.init]p16, second bullet to class types, this initialization + // Perform overload resolution using the class's constructors. Per + // C++11 [dcl.init]p16, second bullet for class types, this initialization // is direct-initialization. OverloadCandidateSet CandidateSet(Loc, OverloadCandidateSet::CSK_Normal); - LookupCopyAndMoveConstructors(S, CandidateSet, Class, CurInitExpr); - - bool HadMultipleCandidates = (CandidateSet.size() > 1); + DeclContext::lookup_result Ctors = S.LookupConstructors(Class); OverloadCandidateSet::iterator Best; - switch (CandidateSet.BestViableFunction(S, Loc, Best)) { + switch (ResolveConstructorOverload( + S, Loc, CurInitExpr, CandidateSet, Ctors, Best, + /*CopyInitializing=*/false, /*AllowExplicit=*/true, + /*OnlyListConstructors=*/false, /*IsListInit=*/false, + /*SecondStepOfCopyInit=*/true)) { case OR_Success: break; @@ -5524,6 +5479,8 @@ static ExprResult CopyObject(Sema &S, return ExprError(); } + bool HadMultipleCandidates = CandidateSet.size() > 1; + CXXConstructorDecl *Constructor = cast(Best->Function); SmallVector ConstructorArgs; CurInit.get(); // Ownership transferred into MultiExprArg, below. @@ -5563,6 +5520,31 @@ static ExprResult CopyObject(Sema &S, if (S.CompleteConstructorCall(Constructor, CurInitExpr, Loc, ConstructorArgs)) return ExprError(); + // C++0x [class.copy]p32: + // When certain criteria are met, an implementation is allowed to + // omit the copy/move construction of a class object, even if the + // copy/move constructor and/or destructor for the object have + // side effects. [...] + // - when a temporary class object that has not been bound to a + // reference (12.2) would be copied/moved to a class object + // with the same cv-unqualified type, the copy/move operation + // can be omitted by constructing the temporary object + // directly into the target of the omitted copy/move + // + // Note that the other three bullets are handled elsewhere. Copy + // elision for return statements and throw expressions are handled as part + // of constructor initialization, while copy elision for exception handlers + // is handled by the run-time. + // + // FIXME: If the function parameter is not the same type as the temporary, we + // should still be able to elide the copy, but we don't have a way to + // represent in the AST how much should be elided in this case. + bool Elidable = + CurInitExpr->isTemporaryObject(S.Context, Class) && + S.Context.hasSameUnqualifiedType( + Best->Function->getParamDecl(0)->getType().getNonReferenceType(), + CurInitExpr->getType()); + // Actually perform the constructor call. CurInit = S.BuildCXXConstructExpr(Loc, T, Best->FoundDecl, Constructor, Elidable, @@ -5598,12 +5580,16 @@ static void CheckCXX98CompatAccessibleCopy(Sema &S, // Find constructors which would have been considered. OverloadCandidateSet CandidateSet(Loc, OverloadCandidateSet::CSK_Normal); - LookupCopyAndMoveConstructors( - S, CandidateSet, cast(Record->getDecl()), CurInitExpr); + DeclContext::lookup_result Ctors = + S.LookupConstructors(cast(Record->getDecl())); // Perform overload resolution. OverloadCandidateSet::iterator Best; - OverloadingResult OR = CandidateSet.BestViableFunction(S, Loc, Best); + OverloadingResult OR = ResolveConstructorOverload( + S, Loc, CurInitExpr, CandidateSet, Ctors, Best, + /*CopyInitializing=*/false, /*AllowExplicit=*/true, + /*OnlyListConstructors=*/false, /*IsListInit=*/false, + /*SecondStepOfCopyInit=*/true); PartialDiagnostic Diag = S.PDiag(diag::warn_cxx98_compat_temp_copy) << OR << (int)Entity.getKind() << CurInitExpr->getType() @@ -5723,9 +5709,10 @@ PerformConstructorInitialization(Sema &S, // T as its first argument, called with a single argument in the // context of direct-initialization, explicit conversion functions // are also considered. - bool AllowExplicitConv = Kind.AllowExplicit() && !Kind.isCopyInit() && - Args.size() == 1 && - Constructor->isCopyOrMoveConstructor(); + bool AllowExplicitConv = + Kind.AllowExplicit() && !Kind.isCopyInit() && Args.size() == 1 && + hasCopyOrMoveCtorParam(S.Context, + getConstructorInfo(Step.Function.FoundDecl)); // Determine the arguments required to actually perform the constructor // call. diff --git a/clang/test/CXX/dcl.decl/dcl.init/dcl.init.ref/p5-cxx03-extra-copy.cpp b/clang/test/CXX/dcl.decl/dcl.init/dcl.init.ref/p5-cxx03-extra-copy.cpp index 494cfb5..7a5caef 100644 --- a/clang/test/CXX/dcl.decl/dcl.init/dcl.init.ref/p5-cxx03-extra-copy.cpp +++ b/clang/test/CXX/dcl.decl/dcl.init/dcl.init.ref/p5-cxx03-extra-copy.cpp @@ -19,7 +19,7 @@ private: }; struct X3 { - X3(); + X3(); // expected-note{{requires 0 arguments, but 1 was provided}} private: X3(X3&); // expected-note{{candidate constructor not viable: expects an l-value for 1st argument}} @@ -42,8 +42,8 @@ struct X4 { // Check for "dangerous" default arguments that could cause recursion. struct X5 { - X5(); - X5(const X5&, const X5& = X5()); // expected-warning{{no viable constructor copying parameter of type 'X5'}} + X5(); // expected-note {{requires 0 arguments}} + X5(const X5&, const X5& = X5()); // expected-warning{{no viable constructor copying parameter of type 'X5'}} expected-note {{requires 2 arguments}} }; void g1(const X1&); diff --git a/clang/test/CXX/drs/dr0xx.cpp b/clang/test/CXX/drs/dr0xx.cpp index 3bb6701..69cb776 100644 --- a/clang/test/CXX/drs/dr0xx.cpp +++ b/clang/test/CXX/drs/dr0xx.cpp @@ -643,8 +643,8 @@ namespace dr58 { // dr58: yes namespace dr59 { // dr59: yes template struct convert_to { operator T() const; }; - struct A {}; // expected-note 2{{volatile qualifier}} - struct B : A {}; // expected-note 2{{volatile qualifier}} + struct A {}; // expected-note 2{{volatile qualifier}} expected-note 2{{requires 0 arguments}} + struct B : A {}; // expected-note 2{{volatile qualifier}} expected-note 2{{requires 0 arguments}} #if __cplusplus >= 201103L // move constructors // expected-note@-3 2{{volatile qualifier}} // expected-note@-3 2{{volatile qualifier}} @@ -902,7 +902,7 @@ namespace dr84 { // dr84: yes struct C {}; struct B { B(B&); // expected-note {{candidate}} - B(C); + B(C); // expected-note {{no known conversion from 'dr84::B' to 'dr84::C'}} operator C() const; }; A a; diff --git a/clang/test/CXX/drs/dr1xx.cpp b/clang/test/CXX/drs/dr1xx.cpp index 8d368a5..60d6cca 100644 --- a/clang/test/CXX/drs/dr1xx.cpp +++ b/clang/test/CXX/drs/dr1xx.cpp @@ -827,7 +827,7 @@ namespace dr177 { // dr177: yes struct B {}; struct A { A(A &); // expected-note {{not viable: expects an l-value}} - A(const B &); + A(const B &); // expected-note {{not viable: no known conversion from 'dr177::A' to}} }; B b; A a = b; // expected-error {{no viable constructor copying variable}} diff --git a/clang/test/CodeGenCXX/copy-constructor-elim-2.cpp b/clang/test/CodeGenCXX/copy-constructor-elim-2.cpp index c263b7e..26b6b48 100644 --- a/clang/test/CodeGenCXX/copy-constructor-elim-2.cpp +++ b/clang/test/CodeGenCXX/copy-constructor-elim-2.cpp @@ -74,3 +74,23 @@ namespace PR12139 { return a.value; } } + +namespace ElidableCallIsNotCopyCtor { + struct A { A(const A&); }; + struct B : A { + B(B&); + B(A); + B(int); + }; + void f() { + // Here, we construct via B(int) then B(A). The B(A) construction is + // elidable, but we don't have an AST representation for the case where we + // must elide not only a constructor call but also some argument + // conversions, so we don't elide it. + // CHECK-LABEL: define void @_ZN25ElidableCallIsNotCopyCtor1fEv( + // CHECK: call {{.*}} @_ZN25ElidableCallIsNotCopyCtor1BC1Ei( + // CHECK: call {{.*}} @_ZN25ElidableCallIsNotCopyCtor1AC1ERKS0_( + // CHECK: call {{.*}} @_ZN25ElidableCallIsNotCopyCtor1BC1ENS_1AE( + B b = 0; + } +} diff --git a/clang/test/SemaCXX/conditional-expr.cpp b/clang/test/SemaCXX/conditional-expr.cpp index c12efc0..5025990 100644 --- a/clang/test/SemaCXX/conditional-expr.cpp +++ b/clang/test/SemaCXX/conditional-expr.cpp @@ -262,7 +262,7 @@ namespace PR6757 { struct Foo2 { }; struct Foo3 { - Foo3(); + Foo3(); // expected-note{{requires 0 arguments}} Foo3(Foo3&); // expected-note{{would lose const qualifier}} }; diff --git a/clang/test/SemaCXX/copy-initialization.cpp b/clang/test/SemaCXX/copy-initialization.cpp index d219ee5..4f6c65c 100644 --- a/clang/test/SemaCXX/copy-initialization.cpp +++ b/clang/test/SemaCXX/copy-initialization.cpp @@ -30,7 +30,7 @@ void test(const foo *P) { P->bar(); } // expected-error{{'bar' not viable: 'this namespace PR6757 { struct Foo { - Foo(); + Foo(); // expected-note{{not viable}} Foo(Foo&); // expected-note{{candidate constructor not viable}} }; @@ -71,3 +71,12 @@ namespace DR5 { const S b = 0; } } + +struct A {}; +struct B : A { + B(); + B(B&); + B(A); + B(int); +}; +B b = 0; // ok, calls B(int) then A(const A&) then B(A). diff --git a/clang/test/SemaCXX/cxx0x-initializer-constructor.cpp b/clang/test/SemaCXX/cxx0x-initializer-constructor.cpp index 6202bf6..c10bee9 100644 --- a/clang/test/SemaCXX/cxx0x-initializer-constructor.cpp +++ b/clang/test/SemaCXX/cxx0x-initializer-constructor.cpp @@ -267,8 +267,11 @@ namespace PR12120 { struct A { explicit A(int); A(float); }; // expected-note {{declared here}} A a = { 0 }; // expected-error {{constructor is explicit}} - struct B { explicit B(short); B(long); }; // expected-note 2 {{candidate}} + struct B { explicit B(short); B(long); }; // expected-note 4{{candidate}} B b = { 0 }; // expected-error {{ambiguous}} + + struct C { explicit C(short); C(long); }; // expected-note 2{{candidate}} + C c = {{ 0 }}; // expected-error {{ambiguous}} } namespace PR12498 { diff --git a/clang/test/SemaCXX/cxx11-inheriting-ctors.cpp b/clang/test/SemaCXX/cxx11-inheriting-ctors.cpp index 9c33ac0..5ce8d1a 100644 --- a/clang/test/SemaCXX/cxx11-inheriting-ctors.cpp +++ b/clang/test/SemaCXX/cxx11-inheriting-ctors.cpp @@ -1,7 +1,5 @@ // RUN: %clang_cc1 -std=c++11 %s -verify -// expected-no-diagnostics - namespace PR15757 { struct S { }; @@ -56,3 +54,33 @@ namespace InvalidConstruction { template int &f(...); int &r = f(0); } + +namespace ExplicitConv { + struct B {}; // expected-note 2{{candidate}} + struct D : B { // expected-note 3{{candidate}} + using B::B; // expected-note 2{{inherited}} + }; + struct X { explicit operator B(); } x; + struct Y { explicit operator D(); } y; + + D dx(x); // expected-error {{no matching constructor}} + D dy(y); +} + +namespace NestedListInit { + struct B { B(); } b; // expected-note 5{{candidate}} + struct D : B { // expected-note 3{{candidate}} + using B::B; // expected-note 2{{inherited}} + }; + // This is a bit weird. We're allowed one pair of braces for overload + // resolution, and one more pair of braces due to [over.ics.list]/2. + B b1 = {b}; + B b2 = {{b}}; + B b3 = {{{b}}}; // expected-error {{no match}} + // This is the same, but we get one call to D's version of B::B(const B&) + // before the two permitted calls to D::D(D&&). + D d1 = {b}; + D d2 = {{b}}; + D d3 = {{{b}}}; + D d4 = {{{{b}}}}; // expected-error {{no match}} +} diff --git a/clang/test/SemaCXX/cxx98-compat-flags.cpp b/clang/test/SemaCXX/cxx98-compat-flags.cpp index f90ad34..62d687c 100644 --- a/clang/test/SemaCXX/cxx98-compat-flags.cpp +++ b/clang/test/SemaCXX/cxx98-compat-flags.cpp @@ -16,7 +16,7 @@ namespace CopyCtorIssues { Private(const Private&); // expected-note {{declared private here}} }; struct NoViable { - NoViable(); + NoViable(); // expected-note {{not viable}} NoViable(NoViable&); // expected-note {{not viable}} }; struct Ambiguous { diff --git a/clang/test/SemaCXX/cxx98-compat-pedantic.cpp b/clang/test/SemaCXX/cxx98-compat-pedantic.cpp index 8b0dcc8..c72c44a 100644 --- a/clang/test/SemaCXX/cxx98-compat-pedantic.cpp +++ b/clang/test/SemaCXX/cxx98-compat-pedantic.cpp @@ -55,7 +55,7 @@ namespace CopyCtorIssues { Private(const Private&); // expected-note {{declared private here}} }; struct NoViable { - NoViable(); + NoViable(); // expected-note {{not viable}} NoViable(NoViable&); // expected-note {{not viable}} }; struct Ambiguous { -- 2.7.4