[PATCH] D41820: [coroutines] Pass coro func args to promise ctor

[Gor Nishanov via Phabricator via cfe-commits]

GorNishanov requested changes to this revision.
GorNishanov added a comment.
This revision now requires changes to proceed.

Thank you for doing this change!



================
Comment at: lib/Sema/SemaCoroutine.cpp:475
 
+// Create a static_cast\<T&&>(expr).
+static Expr *castForMoving(Sema &S, Expr *E, QualType T = QualType()) {
----------------
I would keep this block of functions in their original place. (Or move them here as a separate NFC) commit.

Easier to review what was changed and what was simply moved.


================
Comment at: lib/Sema/SemaCoroutine.cpp:570
+    auto RefExpr = ExprEmpty();
+    auto Moves = ScopeInfo->CoroutineParameterMoves;
+    if (Moves.find(PD) != Moves.end()) {
----------------
This creates a copy of the CoroutineParameterMoves map in every iteration of the loop. 
It seems like an intent is just to create a shorter alias "Moves" to it to refer later.

I suggest:
1) Make Moves a reference to the map: `auto &Moves = ...`
2) Move it out of the loop



================
Comment at: lib/Sema/SemaCoroutine.cpp:572
+    if (Moves.find(PD) != Moves.end()) {
+      // If a reference to the function parameter exists in the coroutine
+      // frame, use that reference.
----------------
Instead of doing lookup twice, once, using `find`,  another, using `operator[]`, you can just say:

```
auto EntryIter = Moves.find(PD);
if (EntryIter != Moves.end()) {
  auto *VD = cast<VarDecl>(cast<DeclStmt>(EntryIter->second)->getSingleDecl());
...
```


================
Comment at: lib/Sema/SemaCoroutine.cpp:574
+      // frame, use that reference.
+      auto *VD = cast<VarDecl>(cast<DeclStmt>(Moves[PD])->getSingleDecl());
+      RefExpr = BuildDeclRefExpr(VD, VD->getType(), ExprValueKind::VK_LValue,
----------------
This VD hides outer VD referring to the promise.
I would rename one of them to some other name.


================
Comment at: lib/Sema/SemaCoroutine.cpp:619
   FD->addDecl(VD);
   assert(!VD->isInvalidDecl());
   return VD;
----------------
I believe this assert needs to be removed. We can get an invalid decl if we failed to find an appropriate constructor. (Couple of negative tests in SemaCXX/coroutines.cpp would help to flush those cases out)


================
Comment at: test/CodeGenCoroutines/coro-alloc.cpp:196
 }
+
+struct promise_matching_constructor {};
----------------
I would move this test to coro-params.cpp, as it is closer to parameter moves than to allocations.

I would also add a negative test or two to SemaCXX/coroutines.cpp to verify that we emit sane errors when something goes wrong with promise constructor and parameter copies.


Repository:
  rC Clang

https://reviews.llvm.org/D41820