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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
