[PATCH] D70555: [coroutines] Don't build promise init with no args

[Brian Gesiak via Phabricator via cfe-commits]

modocache created this revision.
modocache added reviewers: GorNishanov, rsmith, lewissbaker.
Herald added subscribers: cfe-commits, EricWF.
Herald added a project: clang.
modocache edited the summary of this revision.

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 <test.cpp:26:3> col:3 __promise '<dependent type>' callinit
  `-ParenListExpr 0xb514510 <col:3> '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 <test.cpp:26:3> col:3 __promise '<dependent type>'

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70555

Files:
  clang/lib/Sema/SemaCoroutine.cpp


Index: clang/lib/Sema/SemaCoroutine.cpp
===================================================================
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -502,8 +502,9 @@
     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<Expr *, 4> CtorArgExprs;
 
   // Add implicit object parameter.
@@ -519,6 +520,7 @@
     }
   }
 
+  // Add the coroutine function's parameters.
   auto &Moves = ScopeInfo->CoroutineParameterMoves;
   for (auto *PD : FD->parameters()) {
     if (PD->getType()->isDependentType())
@@ -540,28 +542,33 @@
     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);
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D70555.230504.patch
Type: text/x-patch
Size: 3566 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191121/a7efe68f/attachment.bin>