diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -4730,18 +4730,12 @@ Status S; bool isParenthesized; - bool isMoveEligible() const { return S >= MoveEligible; }; bool isCopyElidable() const { return S == MoveEligibleAndCopyElidable; } }; - NRVOResult getNRVOResult(Expr *&E, bool ForceCXX2b = false); - NRVOResult getNRVOResult(const VarDecl *VD, bool Parenthesized = false, - bool ForceCXX20 = false); + NRVOResult getNRVOResult(Expr *&E); + NRVOResult getNRVOResult(const VarDecl *VD, bool Parenthesized = false); void updNRVOResultWithRetType(NRVOResult &Res, QualType ReturnType); - ExprResult PerformMoveOrCopyInitialization(const InitializedEntity &Entity, - const NRVOResult &NRVORes, - Expr *Value); - StmtResult ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp, Scope *CurScope); StmtResult BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp); diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp --- a/clang/lib/Sema/SemaCoroutine.cpp +++ b/clang/lib/Sema/SemaCoroutine.cpp @@ -997,7 +997,7 @@ VarDecl *Promise = FSI->CoroutinePromise; ExprResult PC; if (E && (isa(E) || !E->getType()->isVoidType())) { - getNRVOResult(E, /*ForceCXX2b=*/true); + getNRVOResult(E); PC = buildPromiseCall(*this, Promise, Loc, "return_value", E); } else { E = MakeFullDiscardedValueExpr(E).get(); diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp --- a/clang/lib/Sema/SemaExprCXX.cpp +++ b/clang/lib/Sema/SemaExprCXX.cpp @@ -876,7 +876,7 @@ InitializedEntity Entity = InitializedEntity::InitializeException( OpLoc, ExceptionObjectTy, /*NRVO=*/NRVORes.isCopyElidable()); - ExprResult Res = PerformMoveOrCopyInitialization(Entity, NRVORes, Ex); + ExprResult Res = PerformCopyInitialization(Entity, SourceLocation(), Ex); if (Res.isInvalid()) return ExprError(); Ex = Res.get(); diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp --- a/clang/lib/Sema/SemaStmt.cpp +++ b/clang/lib/Sema/SemaStmt.cpp @@ -3046,25 +3046,22 @@ /// \returns An aggregate which contains the Candidate and isMoveEligible /// and isCopyElidable methods. If Candidate is non-null, it means /// isMoveEligible() would be true under the most permissive language standard. -Sema::NRVOResult Sema::getNRVOResult(const VarDecl *VD, bool Parenthesized, - bool ForceCXX20) { +Sema::NRVOResult Sema::getNRVOResult(const VarDecl *VD, bool Parenthesized) { if (!VD) return NRVOResult(); - bool hasCXX11 = getLangOpts().CPlusPlus11 || ForceCXX20, - hasCXX20 = getLangOpts().CPlusPlus20 || ForceCXX20; NRVOResult Res{VD, NRVOResult::MoveEligibleAndCopyElidable, Parenthesized}; // (other than a function ... parameter) if (VD->getKind() == Decl::ParmVar) { - downgradeNRVOResult(Res, hasCXX11); + downgradeNRVOResult(Res, getLangOpts().CPlusPlus11); } else if (VD->getKind() != Decl::Var) { return NRVOResult(); } // (other than ... a catch-clause parameter) if (VD->isExceptionVariable()) - downgradeNRVOResult(Res, hasCXX20); + downgradeNRVOResult(Res, getLangOpts().CPlusPlus20); // ...automatic... if (!VD->hasLocalStorage()) @@ -3090,7 +3087,7 @@ if (VDReferencedType.isVolatileQualified() || !VDReferencedType->isObjectType()) return NRVOResult(); - downgradeNRVOResult(Res, hasCXX20); + downgradeNRVOResult(Res, getLangOpts().CPlusPlus20); } else { return NRVOResult(); } @@ -3099,7 +3096,7 @@ // alignment cannot use NRVO. if (!VDType->isDependentType() && VD->hasAttr() && Context.getDeclAlign(VD) > Context.getTypeAlignInChars(VDType)) - downgradeNRVOResult(Res, hasCXX11); + downgradeNRVOResult(Res, getLangOpts().CPlusPlus11); return Res; } @@ -3118,7 +3115,7 @@ /// \returns An aggregate which contains the Candidate and isMoveEligible /// and isCopyElidable methods. If Candidate is non-null, it means /// isMoveEligible() would be true under the most permissive language standard. -Sema::NRVOResult Sema::getNRVOResult(Expr *&E, bool ForceCXX2b) { +Sema::NRVOResult Sema::getNRVOResult(Expr *&E) { if (!E) return NRVOResult(); bool Parenthesized = isa(E); @@ -3128,12 +3125,39 @@ if (!DR || DR->refersToEnclosingVariableOrCapture()) return NRVOResult(); const auto *VD = dyn_cast(DR->getDecl()); - NRVOResult Res = getNRVOResult(VD, Parenthesized, /*ForceCXX20=*/ForceCXX2b); - if (Res.Candidate && E->getValueKind() != VK_XValue && - (ForceCXX2b || getLangOpts().CPlusPlus2b)) { - E = ImplicitCastExpr::Create(Context, VD->getType().getNonReferenceType(), - CK_NoOp, E, nullptr, VK_XValue, - FPOptionsOverride()); + NRVOResult Res = getNRVOResult(VD, Parenthesized); + if (Res.S >= NRVOResult::MoveEligible) { + ExprValueKind CastToKind = + getLangOpts().CPlusPlus2b ? VK_XValue : VK_XValue; + if (E->getValueKind() != CastToKind) { + E = ImplicitCastExpr::Create(Context, VD->getType().getNonReferenceType(), + CK_NoOp, E, nullptr, CastToKind, + FPOptionsOverride()); + } + } else if (Res.Candidate && + !getDiagnostics().isIgnored(diag::warn_return_std_move, + E->getExprLoc())) { + QualType QT = Res.Candidate->getType(); + if (QT->isLValueReferenceType()) { + // Adding 'std::move' around an lvalue reference variable's name is + // dangerous. Don't suggest it. + } else if (QT.getNonReferenceType() + .getUnqualifiedType() + .isTriviallyCopyableType(Context)) { + // Adding 'std::move' around a trivially copyable variable is probably + // pointless. Don't suggest it. + } else { + bool IsThrow = + false; //(Entity.getKind() == InitializedEntity::EK_Exception); + SmallString<32> Str; + Str += "std::move("; + Str += Res.Candidate->getDeclName().getAsString(); + Str += ")"; + Diag(E->getExprLoc(), diag::warn_return_std_move) + << E->getSourceRange() << Res.Candidate->getDeclName() << IsThrow; + Diag(E->getExprLoc(), diag::note_add_std_move) + << FixItHint::CreateReplacement(E->getSourceRange(), Str); + } } return Res; } @@ -3185,160 +3209,6 @@ downgradeNRVOResult(Res, getLangOpts().CPlusPlus11); } -/// Try to perform the initialization of a potentially-movable value, -/// which is the operand to a return or throw statement. -/// -/// This routine implements C++20 [class.copy.elision]p3, 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. -/// -/// \param ConvertingConstructorsOnly If true, follow [class.copy.elision]p3 and -/// reject resolutions that find non-constructors, such as derived-to-base -/// conversions or `operator T()&&` member functions. If false, do consider such -/// conversion sequences. -/// -/// \param Res We will fill this in if move-initialization was possible. -/// If move-initialization is not possible, such that we must fall back to -/// treating the operand as an lvalue, we will leave Res in its original -/// invalid state. -/// -/// \returns Whether we need to do the second overload resolution. If the first -/// overload resolution fails, or if the first overload resolution succeeds but -/// the selected constructor/operator doesn't match the additional criteria, we -/// need to do the second overload resolution. -static bool TryMoveInitialization(Sema &S, const InitializedEntity &Entity, - const VarDecl *NRVOCandidate, Expr *&Value, - bool ConvertingConstructorsOnly, - bool IsDiagnosticsCheck, ExprResult &Res) { - ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(), - CK_NoOp, Value, VK_XValue, FPOptionsOverride()); - - Expr *InitExpr = &AsRvalue; - - InitializationKind Kind = InitializationKind::CreateCopy( - Value->getBeginLoc(), Value->getBeginLoc()); - - InitializationSequence Seq(S, Entity, Kind, InitExpr); - - bool NeedSecondOverloadResolution = true; - if (!Seq && - (IsDiagnosticsCheck || Seq.getFailedOverloadResult() != OR_Deleted)) { - return NeedSecondOverloadResolution; - } - - for (const InitializationSequence::Step &Step : Seq.steps()) { - if (Step.Kind != InitializationSequence::SK_ConstructorInitialization && - Step.Kind != InitializationSequence::SK_UserConversion) - continue; - - FunctionDecl *FD = Step.Function.Function; - if (ConvertingConstructorsOnly) { - if (isa(FD)) { - // C++11 [class.copy]p32: - // C++14 [class.copy]p32: - // C++17 [class.copy.elision]p3: - // [...] 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. - const RValueReferenceType *RRefType = - FD->getParamDecl(0)->getType()->getAs(); - if (!RRefType) - break; - if (!S.Context.hasSameUnqualifiedType(RRefType->getPointeeType(), - NRVOCandidate->getType())) - break; - } else { - continue; - } - } else { - if (isa(FD)) { - // Check that overload resolution selected a constructor taking an - // rvalue reference. If it selected an lvalue reference, then we - // didn't need to cast this thing to an rvalue in the first place. - if (IsDiagnosticsCheck && - !isa(FD->getParamDecl(0)->getType())) - break; - } else if (isa(FD)) { - // Check that overload resolution selected a conversion operator - // taking an rvalue reference. - if (cast(FD)->getRefQualifier() != RQ_RValue) - break; - } else { - continue; - } - } - - NeedSecondOverloadResolution = false; - // Promote "AsRvalue" to the heap, since we now need this - // expression node to persist. - Value = - ImplicitCastExpr::Create(S.Context, Value->getType(), CK_NoOp, Value, - nullptr, VK_XValue, FPOptionsOverride()); - - // Complete type-checking the initialization of the return type - // using the constructor we found. - Res = Seq.Perform(S, Entity, Kind, Value); - } - - return NeedSecondOverloadResolution; -} - -/// Perform the initialization of a potentially-movable value, which -/// is the result of return value. -/// -/// This routine implements C++20 [class.copy.elision]p3, 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. -ExprResult -Sema::PerformMoveOrCopyInitialization(const InitializedEntity &Entity, - const NRVOResult &NRVORes, Expr *Value) { - - if (NRVORes.Candidate && !getLangOpts().CPlusPlus2b) { - if (NRVORes.isMoveEligible()) { - ExprResult Res; - if (!TryMoveInitialization(*this, Entity, NRVORes.Candidate, Value, - !getLangOpts().CPlusPlus20, false, Res)) - return Res; - } - if (!getDiagnostics().isIgnored(diag::warn_return_std_move, - Value->getExprLoc())) { - QualType QT = NRVORes.Candidate->getType(); - if (QT->isLValueReferenceType()) { - // Adding 'std::move' around an lvalue reference variable's name is - // dangerous. Don't suggest it. - } else if (QT.getNonReferenceType() - .getUnqualifiedType() - .isTriviallyCopyableType(Context)) { - // Adding 'std::move' around a trivially copyable variable is probably - // pointless. Don't suggest it. - } else { - ExprResult FakeRes = ExprError(); - Expr *FakeValue = Value; - TryMoveInitialization(*this, Entity, NRVORes.Candidate, FakeValue, - false, true, FakeRes); - if (!FakeRes.isInvalid()) { - bool IsThrow = (Entity.getKind() == InitializedEntity::EK_Exception); - SmallString<32> Str; - Str += "std::move("; - Str += NRVORes.Candidate->getDeclName().getAsString(); - Str += ")"; - Diag(Value->getExprLoc(), diag::warn_return_std_move) - << Value->getSourceRange() << NRVORes.Candidate->getDeclName() - << IsThrow; - Diag(Value->getExprLoc(), diag::note_add_std_move) - << FixItHint::CreateReplacement(Value->getSourceRange(), Str); - } - } - } - } - - // 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. - return PerformCopyInitialization(Entity, SourceLocation(), Value); -} - /// Determine whether the declared return type of the specified function /// contains 'auto'. static bool hasDeducedReturnType(FunctionDecl *FD) { @@ -3483,7 +3353,7 @@ InitializedEntity Entity = InitializedEntity::InitializeResult( ReturnLoc, FnRetType, NRVORes.isCopyElidable()); ExprResult Res = - PerformMoveOrCopyInitialization(Entity, NRVORes, RetValExp); + PerformCopyInitialization(Entity, SourceLocation(), RetValExp); if (Res.isInvalid()) { // FIXME: Cleanup temporaries here, anyway? return StmtError(); @@ -3894,7 +3764,7 @@ InitializedEntity Entity = InitializedEntity::InitializeResult( ReturnLoc, RetType, NRVORes.isCopyElidable()); ExprResult Res = - PerformMoveOrCopyInitialization(Entity, NRVORes, RetValExp); + PerformCopyInitialization(Entity, SourceLocation(), RetValExp); if (Res.isInvalid()) { // FIXME: Clean up temporaries here anyway? return StmtError();