From 627e01feb718dd1aa8e184545976b7229de312a2 Mon Sep 17 00:00:00 2001 From: Brian Gesiak Date: Thu, 21 Nov 2019 13:51:44 -0500 Subject: [PATCH] [coroutines] Don't build promise init with no args Summary: In the case of a coroutine that takes no arguments, `Sema::buildCoroutinePromise` constructs a list-initialization (`clang::InitializationKind::InitKind::IK_DirectList`) of the promise variable, using a list of empty arguments. So, if one were to dump the promise `VarDecl` immediately after `Sema::ActOnCoroutineBodyStart` calls `checkCoroutineContext`, for a coroutine function that takes no arguments, they'd see the following: ``` VarDecl 0xb514490 col:3 __promise '' callinit `-ParenListExpr 0xb514510 'NULL TYPE' ``` But after this patch, the `ParenListExpr` is no longer constructed, and the promise variable uses default initialization (`clang::InitializationKind::InitKind::IK_Default`): ``` VarDecl 0x63100012dae0 col:3 __promise '' ``` As far as I know, there's no case in which list-initialization with no arguments differs from default initialization, but if I'm wrong please let me know (and I'll add a test case that demonstrates the change -- but as-is I can't think of a functional test case for this). I think both comply with the wording of C++20 `[dcl.fct.def.coroutine]p5`: > _promise-constructor-arguments_ is determined as follows: overload resolution is performed on a promise constructor call created by assembling an argument list with lvalues `p1 ... pn`. If a viable constructor is found (12.4.2), then _promise-constructor-arguments_ is `(p1, ... , pn)`, otherwise _promise-constructor-arguments_ is empty. Still, I think this patch is an improvement regardless, because it reduces the size of the AST. Reviewers: GorNishanov, rsmith, lewissbaker Subscribers: EricWF, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D70555 --- clang/lib/Sema/SemaCoroutine.cpp | 55 ++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp index 6dc9e34..5ed0bbd 100644 --- a/clang/lib/Sema/SemaCoroutine.cpp +++ b/clang/lib/Sema/SemaCoroutine.cpp @@ -502,8 +502,9 @@ VarDecl *Sema::buildCoroutinePromise(SourceLocation Loc) { return nullptr; auto *ScopeInfo = getCurFunction(); - // Build a list of arguments, based on the coroutine functions arguments, - // that will be passed to the promise type's constructor. + + // Build a list of arguments, based on the coroutine function's arguments, + // that if present will be passed to the promise type's constructor. llvm::SmallVector CtorArgExprs; // Add implicit object parameter. @@ -519,6 +520,7 @@ VarDecl *Sema::buildCoroutinePromise(SourceLocation Loc) { } } + // Add the coroutine function's parameters. auto &Moves = ScopeInfo->CoroutineParameterMoves; for (auto *PD : FD->parameters()) { if (PD->getType()->isDependentType()) @@ -540,28 +542,33 @@ VarDecl *Sema::buildCoroutinePromise(SourceLocation Loc) { CtorArgExprs.push_back(RefExpr.get()); } - // Create an initialization sequence for the promise type using the - // constructor arguments, wrapped in a parenthesized list expression. - Expr *PLE = ParenListExpr::Create(Context, FD->getLocation(), - CtorArgExprs, FD->getLocation()); - InitializedEntity Entity = InitializedEntity::InitializeVariable(VD); - InitializationKind Kind = InitializationKind::CreateForInit( - VD->getLocation(), /*DirectInit=*/true, PLE); - InitializationSequence InitSeq(*this, Entity, Kind, CtorArgExprs, - /*TopLevelOfInitList=*/false, - /*TreatUnavailableAsInvalid=*/false); - - // Attempt to initialize the promise type with the arguments. - // If that fails, fall back to the promise type's default constructor. - if (InitSeq) { - ExprResult Result = InitSeq.Perform(*this, Entity, Kind, CtorArgExprs); - if (Result.isInvalid()) { - VD->setInvalidDecl(); - } else if (Result.get()) { - VD->setInit(MaybeCreateExprWithCleanups(Result.get())); - VD->setInitStyle(VarDecl::CallInit); - CheckCompleteVariableDeclaration(VD); - } + // If we have a non-zero number of constructor arguments, try to use them. + // Otherwise, fall back to the promise type's default constructor. + if (!CtorArgExprs.empty()) { + // Create an initialization sequence for the promise type using the + // constructor arguments, wrapped in a parenthesized list expression. + Expr *PLE = ParenListExpr::Create(Context, FD->getLocation(), + CtorArgExprs, FD->getLocation()); + InitializedEntity Entity = InitializedEntity::InitializeVariable(VD); + InitializationKind Kind = InitializationKind::CreateForInit( + VD->getLocation(), /*DirectInit=*/true, PLE); + InitializationSequence InitSeq(*this, Entity, Kind, CtorArgExprs, + /*TopLevelOfInitList=*/false, + /*TreatUnavailableAsInvalid=*/false); + + // Attempt to initialize the promise type with the arguments. + // If that fails, fall back to the promise type's default constructor. + if (InitSeq) { + ExprResult Result = InitSeq.Perform(*this, Entity, Kind, CtorArgExprs); + if (Result.isInvalid()) { + VD->setInvalidDecl(); + } else if (Result.get()) { + VD->setInit(MaybeCreateExprWithCleanups(Result.get())); + VD->setInitStyle(VarDecl::CallInit); + CheckCompleteVariableDeclaration(VD); + } + } else + ActOnUninitializedDecl(VD); } else ActOnUninitializedDecl(VD); -- 2.7.4