From 627e01feb718dd1aa8e184545976b7229de312a2 Mon Sep 17 00:00:00 2001
From: Brian Gesiak <modocache@gmail.com>
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 <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.

Reviewers: GorNishanov, rsmith, lewissbaker

Subscribers: EricWF, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D70555
---
 clang/lib/Sema/SemaCoroutine.cpp | 53 ++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/clang/lib/Sema/SemaCoroutine.cpp b/clang/lib/Sema/SemaCoroutine.cpp
index 6dc9e342beb9..5ed0bbd6041d 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<Expr *, 4> 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);
+  // 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);
-    }
+    // 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);