Skip to content

Commit d9308aa

Browse files
mizvekovmizvekov
committed
[clang] don't mark as Elidable CXXConstruct expressions used in NRVO
See PR51862. The consumers of the Elidable flag in CXXConstructExpr assume that an elidable construction just goes through a single copy/move construction, so that the source object is immediately passed as an argument and is the same type as the parameter itself. With the implementation of P2266 and after some adjustments to the implementation of P1825, we started (correctly, as per standard) allowing more cases where the copy initialization goes through user defined conversions. With this patch we stop using this flag in NRVO contexts, to preserve code that relies on that assumption. This causes no known functional changes, we just stop firing some asserts in a cople of included test cases. Reviewed By: rsmith Differential Revision: https://reviews.llvm.org/D109800
1 parent 33e1713 commit d9308aa

File tree

13 files changed

+122
-33
lines changed

13 files changed

+122
-33
lines changed

‎clang/include/clang/Sema/Initialization.h

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -298,8 +298,8 @@ class alignas(8) InitializedEntity {
298298

299299
/// Create the initialization entity for the result of a function.
300300
static InitializedEntity InitializeResult(SourceLocation ReturnLoc,
301-
QualType Type, bool NRVO) {
302-
return InitializedEntity(EK_Result, ReturnLoc, Type, NRVO);
301+
QualType Type) {
302+
return InitializedEntity(EK_Result, ReturnLoc, Type);
303303
}
304304

305305
static InitializedEntity InitializeStmtExprResult(SourceLocation ReturnLoc,
@@ -308,20 +308,20 @@ class alignas(8) InitializedEntity {
308308
}
309309

310310
static InitializedEntity InitializeBlock(SourceLocation BlockVarLoc,
311-
QualType Type, bool NRVO) {
312-
return InitializedEntity(EK_BlockElement, BlockVarLoc, Type, NRVO);
311+
QualType Type) {
312+
return InitializedEntity(EK_BlockElement, BlockVarLoc, Type);
313313
}
314314

315315
static InitializedEntity InitializeLambdaToBlock(SourceLocation BlockVarLoc,
316-
QualType Type, bool NRVO) {
316+
QualType Type) {
317317
return InitializedEntity(EK_LambdaToBlockConversionBlockElement,
318-
BlockVarLoc, Type, NRVO);
318+
BlockVarLoc, Type);
319319
}
320320

321321
/// Create the initialization entity for an exception object.
322322
static InitializedEntity InitializeException(SourceLocation ThrowLoc,
323-
QualType Type, bool NRVO) {
324-
return InitializedEntity(EK_Exception, ThrowLoc, Type, NRVO);
323+
QualType Type) {
324+
return InitializedEntity(EK_Exception, ThrowLoc, Type);
325325
}
326326

327327
/// Create the initialization entity for an object allocated via new.

‎clang/lib/AST/ExprConstant.cpp

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9933,10 +9933,19 @@ bool RecordExprEvaluator::VisitCXXConstructExpr(const CXXConstructExpr *E,
99339933
return false;
99349934

99359935
// Avoid materializing a temporary for an elidable copy/move constructor.
9936-
if (E->isElidable() && !ZeroInit)
9937-
if (const MaterializeTemporaryExpr *ME
9938-
= dyn_cast<MaterializeTemporaryExpr>(E->getArg(0)))
9936+
if (E->isElidable() && !ZeroInit) {
9937+
// FIXME: This only handles the simplest case, where the source object
9938+
// is passed directly as the first argument to the constructor.
9939+
// This should also handle stepping though implicit casts and
9940+
// and conversion sequences which involve two steps, with a
9941+
// conversion operator followed by a converting constructor.
9942+
const Expr *SrcObj = E->getArg(0);
9943+
assert(SrcObj->isTemporaryObject(Info.Ctx, FD->getParent()));
9944+
assert(Info.Ctx.hasSameUnqualifiedType(E->getType(), SrcObj->getType()));
9945+
if (const MaterializeTemporaryExpr *ME =
9946+
dyn_cast<MaterializeTemporaryExpr>(SrcObj))
99399947
return Visit(ME->getSubExpr());
9948+
}
99409949

99419950
if (ZeroInit && !ZeroInitialization(E, T))
99429951
return false;

‎clang/lib/CodeGen/CGExprCXX.cpp

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -609,15 +609,18 @@ CodeGenFunction::EmitCXXConstructExpr(const CXXConstructExpr *E,
609609
return;
610610

611611
// Elide the constructor if we're constructing from a temporary.
612-
// The temporary check is required because Sema sets this on NRVO
613-
// returns.
614612
if (getLangOpts().ElideConstructors && E->isElidable()) {
615-
assert(getContext().hasSameUnqualifiedType(E->getType(),
616-
E->getArg(0)->getType()));
617-
if (E->getArg(0)->isTemporaryObject(getContext(), CD->getParent())) {
618-
EmitAggExpr(E->getArg(0), Dest);
619-
return;
620-
}
613+
// FIXME: This only handles the simplest case, where the source object
614+
// is passed directly as the first argument to the constructor.
615+
// This should also handle stepping though implicit casts and
616+
// conversion sequences which involve two steps, with a
617+
// conversion operator followed by a converting constructor.
618+
const Expr *SrcObj = E->getArg(0);
619+
assert(SrcObj->isTemporaryObject(getContext(), CD->getParent()));
620+
assert(
621+
getContext().hasSameUnqualifiedType(E->getType(), SrcObj->getType()));
622+
EmitAggExpr(SrcObj, Dest);
623+
return;
621624
}
622625

623626
if (const ArrayType *arrayType

‎clang/lib/Sema/Sema.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2022,7 +2022,7 @@ static void checkEscapingByref(VarDecl *VD, Sema &S) {
20222022
Expr *VarRef =
20232023
new (S.Context) DeclRefExpr(S.Context, VD, false, T, VK_LValue, Loc);
20242024
ExprResult Result;
2025-
auto IE = InitializedEntity::InitializeBlock(Loc, T, false);
2025+
auto IE = InitializedEntity::InitializeBlock(Loc, T);
20262026
if (S.getLangOpts().CPlusPlus2b) {
20272027
auto *E = ImplicitCastExpr::Create(S.Context, T, CK_NoOp, VarRef, nullptr,
20282028
VK_XValue, FPOptionsOverride());

‎clang/lib/Sema/SemaCoroutine.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1533,7 +1533,7 @@ bool CoroutineStmtBuilder::makeGroDeclAndReturnStmt() {
15331533
if (GroType->isVoidType()) {
15341534
// Trigger a nice error message.
15351535
InitializedEntity Entity =
1536-
InitializedEntity::InitializeResult(Loc, FnRetType, false);
1536+
InitializedEntity::InitializeResult(Loc, FnRetType);
15371537
S.PerformCopyInitialization(Entity, SourceLocation(), ReturnValue);
15381538
noteMemberDeclaredHere(S, ReturnValue, Fn);
15391539
return false;

‎clang/lib/Sema/SemaDeclCXX.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15312,8 +15312,17 @@ Sema::BuildCXXConstructExpr(SourceLocation ConstructLoc, QualType DeclInitType,
1531215312
// can be omitted by constructing the temporary object
1531315313
// directly into the target of the omitted copy/move
1531415314
if (ConstructKind == CXXConstructExpr::CK_Complete && Constructor &&
15315+
// FIXME: Converting constructors should also be accepted.
15316+
// But to fix this, the logic that digs down into a CXXConstructExpr
15317+
// to find the source object needs to handle it.
15318+
// Right now it assumes the source object is passed directly as the
15319+
// first argument.
1531515320
Constructor->isCopyOrMoveConstructor() && hasOneRealArgument(ExprArgs)) {
1531615321
Expr *SubExpr = ExprArgs[0];
15322+
// FIXME: Per above, this is also incorrect if we want to accept
15323+
// converting constructors, as isTemporaryObject will
15324+
// reject temporaries with different type from the
15325+
// CXXRecord itself.
1531715326
Elidable = SubExpr->isTemporaryObject(
1531815327
Context, cast<CXXRecordDecl>(FoundDecl->getDeclContext()));
1531915328
}

‎clang/lib/Sema/SemaExpr.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15713,7 +15713,7 @@ ExprResult Sema::ActOnBlockStmtExpr(SourceLocation CaretLoc,
1571315713
if (!Result.isInvalid()) {
1571415714
Result = PerformCopyInitialization(
1571515715
InitializedEntity::InitializeBlock(Var->getLocation(),
15716-
Cap.getCaptureType(), false),
15716+
Cap.getCaptureType()),
1571715717
Loc, Result.get());
1571815718
}
1571915719

‎clang/lib/Sema/SemaExprCXX.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -893,9 +893,8 @@ ExprResult Sema::BuildCXXThrow(SourceLocation OpLoc, Expr *Ex,
893893
if (CheckCXXThrowOperand(OpLoc, ExceptionObjectTy, Ex))
894894
return ExprError();
895895

896-
InitializedEntity Entity = InitializedEntity::InitializeException(
897-
OpLoc, ExceptionObjectTy,
898-
/*NRVO=*/NRInfo.isCopyElidable());
896+
InitializedEntity Entity =
897+
InitializedEntity::InitializeException(OpLoc, ExceptionObjectTy);
899898
ExprResult Res = PerformMoveOrCopyInitialization(Entity, NRInfo, Ex);
900899
if (Res.isInvalid())
901900
return ExprError();

‎clang/lib/Sema/SemaLambda.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1978,8 +1978,7 @@ ExprResult Sema::BuildBlockForLambdaConversion(SourceLocation CurrentLocation,
19781978
CallOperator->markUsed(Context);
19791979

19801980
ExprResult Init = PerformCopyInitialization(
1981-
InitializedEntity::InitializeLambdaToBlock(ConvLocation, Src->getType(),
1982-
/*NRVO=*/false),
1981+
InitializedEntity::InitializeLambdaToBlock(ConvLocation, Src->getType()),
19831982
CurrentLocation, Src);
19841983
if (!Init.isInvalid())
19851984
Init = ActOnFinishFullExpr(Init.get(), /*DiscardedValue*/ false);

‎clang/lib/Sema/SemaObjCProperty.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1467,8 +1467,7 @@ Decl *Sema::ActOnPropertyImplDecl(Scope *S,
14671467
LoadSelfExpr, true, true);
14681468
ExprResult Res = PerformCopyInitialization(
14691469
InitializedEntity::InitializeResult(PropertyDiagLoc,
1470-
getterMethod->getReturnType(),
1471-
/*NRVO=*/false),
1470+
getterMethod->getReturnType()),
14721471
PropertyDiagLoc, IvarRefExpr);
14731472
if (!Res.isInvalid()) {
14741473
Expr *ResExpr = Res.getAs<Expr>();

0 commit comments

Comments
 (0)