[PATCH] D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves

[Brian Gesiak via Phabricator via cfe-commits]

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

Sorry for the slow response here, @junparser!

The test case you came up with here is great! I can see the issue is that `ScopeInfo->CoroutineParameterMoves` are built up when Clang parses the first `co_await a`, but are not cleared when `lookupPromiseType` results in an error. As a result, when Clang hits the second `co_await a`, it's in a state that the current code didn't anticipate. Your test case does a great job demonstrating this. Your fix for the problem also looks good to me! My only suggestion is to make the test case just a little clearer, as I'll explain...

(Also, in the future could you please upload your patches with full context? You can read https://llvm.org/docs/Phabricator.html for more details. I think the section explaining the web interface might be relevant to you, where it suggests `git show HEAD -U999999 > mypatch.patch`. The reason I ask is because on Phabricator I can see what lines you're proposing should be added, but not the surrounding source lines, which made this more difficult to review.)



================
Comment at: test/SemaCXX/coroutines.cpp:93
+  co_await a;
+}
+
----------------
This is a great test case, thanks for coming up with it! However, I think it could be a little clearer, though: right now the `int a` variable is shadowing the `awaitable a` that's declared further up in this file. At first, I wasn't sure whether the shadowing had something to do with the test, but in fact it doesn't. This test case demonstrates the issue but without the confusion, I think:

```
int no_promise_type_multiple_awaits(int) { // expected-error {{this function cannot be a coroutine: 'std::experimental::coroutine_traits<int, int>' has no member named 'promise_type'}}
  co_await a;
  co_await a;
}
```


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69022/new/

https://reviews.llvm.org/D69022