From: Douglas Gregor Date: Fri, 21 Jan 2011 19:38:21 +0000 (+0000) Subject: Implement the preference for move-construction over copy-construction X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=cc15f010672a13b38104a32e3cefc7adc07ffbf7;p=clang Implement the preference for move-construction over copy-construction when returning an NRVO candidate expression. For example, this properly picks the move constructor when dealing with code such as MoveOnlyType f() { MoveOnlyType mot; return mot; } The previously-XFAIL'd rvalue-references test case now works, and has been moved into the appropriate paragraph-specific test case. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@123992 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/include/clang/AST/DeclCXX.h b/include/clang/AST/DeclCXX.h index 9dee1b5dc9..ffa459e673 100644 --- a/include/clang/AST/DeclCXX.h +++ b/include/clang/AST/DeclCXX.h @@ -1473,6 +1473,29 @@ public: return isCopyConstructor(TypeQuals); } + /// \brief Determine whether this constructor is a move constructor + /// (C++0x [class.copy]p3), which can be used to move values of the class. + /// + /// \param TypeQuals If this constructor is a move constructor, will be set + /// to the type qualifiers on the referent of the first parameter's type. + bool isMoveConstructor(unsigned &TypeQuals) const; + + /// \brief Determine whether this constructor is a move constructor + /// (C++0x [class.copy]p3), which can be used to move values of the class. + bool isMoveConstructor() const; + + /// \brief Determine whether this is a copy or move constructor. + /// + /// \param TypeQuals Will be set to the type qualifiers on the reference + /// parameter, if in fact this is a copy or move constructor. + bool isCopyOrMoveConstructor(unsigned &TypeQuals) const; + + /// \brief Determine whether this a copy or move constructor. + bool isCopyOrMoveConstructor() const { + unsigned Quals; + return isCopyOrMoveConstructor(Quals); + } + /// isConvertingConstructor - Whether this constructor is a /// converting constructor (C++ [class.conv.ctor]), which can be /// used for user-defined conversions. diff --git a/lib/AST/DeclCXX.cpp b/lib/AST/DeclCXX.cpp index 4548273333..334ee795f3 100644 --- a/lib/AST/DeclCXX.cpp +++ b/lib/AST/DeclCXX.cpp @@ -1116,25 +1116,40 @@ bool CXXConstructorDecl::isDefaultConstructor() const { bool CXXConstructorDecl::isCopyConstructor(unsigned &TypeQuals) const { + return isCopyOrMoveConstructor(TypeQuals) && + getParamDecl(0)->getType()->isLValueReferenceType(); +} + +bool CXXConstructorDecl::isMoveConstructor(unsigned &TypeQuals) const { + return isCopyOrMoveConstructor(TypeQuals) && + getParamDecl(0)->getType()->isRValueReferenceType(); +} + +/// \brief Determine whether this is a copy or move constructor. +bool CXXConstructorDecl::isCopyOrMoveConstructor(unsigned &TypeQuals) const { // C++ [class.copy]p2: // A non-template constructor for class X is a copy constructor // if its first parameter is of type X&, const X&, volatile X& or // const volatile X&, and either there are no other parameters // or else all other parameters have default arguments (8.3.6). + // C++0x [class.copy]p3: + // A non-template constructor for class X is a move constructor if its + // first parameter is of type X&&, const X&&, volatile X&&, or + // const volatile X&&, and either there are no other parameters or else + // all other parameters have default arguments. if ((getNumParams() < 1) || (getNumParams() > 1 && !getParamDecl(1)->hasDefaultArg()) || (getPrimaryTemplate() != 0) || (getDescribedFunctionTemplate() != 0)) return false; - + const ParmVarDecl *Param = getParamDecl(0); - - // Do we have a reference type? Rvalue references don't count. - const LValueReferenceType *ParamRefType = - Param->getType()->getAs(); + + // Do we have a reference type? + const ReferenceType *ParamRefType = Param->getType()->getAs(); if (!ParamRefType) return false; - + // Is it a reference to our class type? ASTContext &Context = getASTContext(); @@ -1144,12 +1159,12 @@ CXXConstructorDecl::isCopyConstructor(unsigned &TypeQuals) const { = Context.getCanonicalType(Context.getTagDeclType(getParent())); if (PointeeType.getUnqualifiedType() != ClassTy) return false; - + // FIXME: other qualifiers? - - // We have a copy constructor. + + // We have a copy or move constructor. TypeQuals = PointeeType.getCVRQualifiers(); - return true; + return true; } bool CXXConstructorDecl::isConvertingConstructor(bool AllowExplicit) const { diff --git a/lib/Sema/SemaInit.cpp b/lib/Sema/SemaInit.cpp index 9e8a15739f..00a13b54cf 100644 --- a/lib/Sema/SemaInit.cpp +++ b/lib/Sema/SemaInit.cpp @@ -3363,20 +3363,20 @@ static ExprResult CopyObject(Sema &S, if (S.RequireCompleteType(Loc, T, S.PDiag(diag::err_temp_copy_incomplete))) return move(CurInit); - // Perform overload resolution using the class's copy constructors. + // Perform overload resolution using the class's copy/move constructors. DeclContext::lookup_iterator Con, ConEnd; OverloadCandidateSet CandidateSet(Loc); for (llvm::tie(Con, ConEnd) = S.LookupConstructors(Class); Con != ConEnd; ++Con) { - // Only consider copy constructors and constructor templates. Per + // Only consider copy/move constructors and constructor templates. Per // C++0x [dcl.init]p16, second bullet to class types, this // initialization is direct-initialization. CXXConstructorDecl *Constructor = 0; if ((Constructor = dyn_cast(*Con))) { - // Handle copy constructors, only. + // Handle copy/moveconstructors, only. if (!Constructor || Constructor->isInvalidDecl() || - !Constructor->isCopyConstructor() || + !Constructor->isCopyOrMoveConstructor() || !Constructor->isConvertingConstructor(/*AllowExplicit=*/true)) continue; diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp index 7010d48d1c..015dcbb8c9 100644 --- a/lib/Sema/SemaStmt.cpp +++ b/lib/Sema/SemaStmt.cpp @@ -1137,6 +1137,86 @@ const VarDecl *Sema::getCopyElisionCandidate(QualType ReturnType, return 0; } +/// \brief Perform the initialization of a return value. +/// +/// This routine implements C++0x [class.copy]p33, which attempts to treat +/// returned lvalues as rvalues in certain cases (to prefer move construction), +/// then falls back to treating them as lvalues if that failed. +static ExprResult initializeReturnValue(Sema &S, + const VarDecl *NRVOCandidate, + SourceLocation ReturnLoc, + QualType ResultType, + Expr *RetValExp) { + // C++0x [class.copy]p33: + // When the criteria for elision of a copy operation are met or would + // be met save for the fact that the source object is a function + // parameter, and the object to be copied is designated by an lvalue, + // overload resolution to select the constructor for the copy is first + // performed as if the object were designated by an rvalue. + InitializedEntity Entity = InitializedEntity::InitializeResult(ReturnLoc, + ResultType, + NRVOCandidate != 0); + + ExprResult Res = ExprError(); + if (NRVOCandidate || S.getCopyElisionCandidate(ResultType, RetValExp, true)) { + ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, + RetValExp->getType(), CK_LValueToRValue, + RetValExp, VK_XValue); + + Expr *InitExpr = &AsRvalue; + InitializationKind Kind + = InitializationKind::CreateCopy(RetValExp->getLocStart(), + RetValExp->getLocStart()); + InitializationSequence Seq(S, Entity, Kind, &InitExpr, 1); + + // [...] If overload resolution fails, or if the type of the first + // parameter of the selected constructor is not an rvalue reference + // to the object’s type (possibly cv-qualified), overload resolution + // is performed again, considering the object as an lvalue. + if (Seq.getKind() != InitializationSequence::FailedSequence) { + for (InitializationSequence::step_iterator Step = Seq.step_begin(), + StepEnd = Seq.step_end(); + Step != StepEnd; ++Step) { + if (Step->Kind + != InitializationSequence::SK_ConstructorInitialization) + continue; + + CXXConstructorDecl *Constructor + = cast(Step->Function.Function); + + const RValueReferenceType *RRefType + = Constructor->getParamDecl(0)->getType() + ->getAs(); + + // If we don't meet the criteria, break out now. + if (!RRefType || + !S.Context.hasSameUnqualifiedType(RRefType->getPointeeType(), + ResultType)) + break; + + // Promote "AsRvalue" to the heap, since we now need this + // expression node to persist. + RetValExp = ImplicitCastExpr::Create(S.Context, + RetValExp->getType(), + CK_LValueToRValue, + RetValExp, 0, VK_XValue); + + // Complete type-checking the initialization of the return type + // using the constructor we found. + Res = Seq.Perform(S, Entity, Kind, MultiExprArg(&RetValExp, 1)); + } + } + } + + // Either we didn't meet the criteria for treating an lvalue as an rvalue, + // above, or overload resolution failed. Either way, we need to try + // (again) now with the return value expression as written. + if (Res.isInvalid()) + Res = S.PerformCopyInitialization(Entity, SourceLocation(), RetValExp); + + return Res; +} + /// ActOnBlockReturnStmt - Utility routine to figure out block's return type. /// StmtResult @@ -1193,12 +1273,8 @@ Sema::ActOnBlockReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) { // In C++ the return statement is handled via a copy initialization. // the C version of which boils down to CheckSingleAssignmentConstraints. NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, false); - ExprResult Res = PerformCopyInitialization( - InitializedEntity::InitializeResult(ReturnLoc, - FnRetType, - NRVOCandidate != 0), - SourceLocation(), - Owned(RetValExp)); + ExprResult Res = initializeReturnValue(*this, NRVOCandidate, ReturnLoc, + FnRetType, RetValExp); if (Res.isInvalid()) { // FIXME: Cleanup temporaries here, anyway? return StmtError(); @@ -1291,12 +1367,8 @@ Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) { // In C++ the return statement is handled via a copy initialization. // the C version of which boils down to CheckSingleAssignmentConstraints. NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, false); - ExprResult Res = PerformCopyInitialization( - InitializedEntity::InitializeResult(ReturnLoc, - FnRetType, - NRVOCandidate != 0), - SourceLocation(), - Owned(RetValExp)); + ExprResult Res = initializeReturnValue(*this, NRVOCandidate, ReturnLoc, + FnRetType, RetValExp); if (Res.isInvalid()) { // FIXME: Cleanup temporaries here, anyway? return StmtError(); diff --git a/test/CXX/special/class.copy/p33-0x.cpp b/test/CXX/special/class.copy/p33-0x.cpp new file mode 100644 index 0000000000..a9ce58937a --- /dev/null +++ b/test/CXX/special/class.copy/p33-0x.cpp @@ -0,0 +1,19 @@ +// RUN: %clang_cc1 -std=c++0x -fsyntax-only -verify %s +class X { + X(const X&); + +public: + X(); + X(X&&); +}; + +X return_by_move(int i, X x) { + X x2; + if (i == 0) + return x; + else if (i == 1) + return x2; + else + return x; +} + diff --git a/test/SemaCXX/rval-references-examples.cpp b/test/SemaCXX/rval-references-examples.cpp index 1cde5854ba..778924d657 100644 --- a/test/SemaCXX/rval-references-examples.cpp +++ b/test/SemaCXX/rval-references-examples.cpp @@ -59,7 +59,7 @@ unique_ptr make_unique_ptr(Args &&...args) { template void accept_unique_ptr(unique_ptr); // expected-note{{passing argument to parameter here}} -void test_unique_ptr() { +unique_ptr test_unique_ptr() { // Simple construction unique_ptr p; unique_ptr p1(new int); @@ -85,4 +85,6 @@ void test_unique_ptr() { // Implicit copies (failures); accept_unique_ptr(p); // expected-error{{call to deleted constructor of 'unique_ptr'}} + + return p; } diff --git a/test/SemaCXX/rval-references-xfail.cpp b/test/SemaCXX/rval-references-xfail.cpp deleted file mode 100644 index d41f8f141c..0000000000 --- a/test/SemaCXX/rval-references-xfail.cpp +++ /dev/null @@ -1,14 +0,0 @@ -// RUN: %clang_cc1 -fsyntax-only -verify -std=c++0x %s -// XFAIL: * -struct MoveOnly { - MoveOnly(); - MoveOnly(const MoveOnly&) = delete; // expected-note {{candidate function}} \ - // expected-note 3{{explicitly marked deleted here}} - MoveOnly(MoveOnly&&); // expected-note {{candidate function}} - MoveOnly(int&&); // expected-note {{candidate function}} -}; - -MoveOnly returning() { - MoveOnly mo; - return mo; -}