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

[Brian Gesiak via Phabricator via cfe-commits]

modocache added a comment.

Thanks again for the patch @junparser! And sorry the review took so long!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Brian Gesiak via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0b3d1d1348da: [coroutines] Remove assert on 
CoroutineParameterMoves in Sema… (authored by modocache).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69022

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/SemaCXX/coroutines.cpp


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -87,6 +87,11 @@
   co_await a;
 }
 
+int no_promise_type_multiple_awaits(int) { // expected-error {{this function 
cannot be a coroutine: 'std::experimental::coroutine_traits' has no 
member named 'promise_type'}}
+  co_await a;
+  co_await a;
+}
+
 template <>
 struct std::experimental::coroutine_traits { typedef int 
promise_type; };
 double bad_promise_type(double) { // expected-error {{this function cannot be 
a coroutine: 'experimental::coroutine_traits::promise_type' 
(aka 'int') is not a class}}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -1527,8 +1527,8 @@
   auto *FD = cast(CurContext);
 
   auto *ScopeInfo = getCurFunction();
-  assert(ScopeInfo->CoroutineParameterMoves.empty() &&
- "Should not build parameter moves twice");
+  if (!ScopeInfo->CoroutineParameterMoves.empty())
+return false;
 
   for (auto *PD : FD->parameters()) {
 if (PD->getType()->isDependentType())


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -87,6 +87,11 @@
   co_await a;
 }
 
+int no_promise_type_multiple_awaits(int) { // expected-error {{this function cannot be a coroutine: 'std::experimental::coroutine_traits' has no member named 'promise_type'}}
+  co_await a;
+  co_await a;
+}
+
 template <>
 struct std::experimental::coroutine_traits { typedef int promise_type; };
 double bad_promise_type(double) { // expected-error {{this function cannot be a coroutine: 'experimental::coroutine_traits::promise_type' (aka 'int') is not a class}}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -1527,8 +1527,8 @@
   auto *FD = cast(CurContext);
 
   auto *ScopeInfo = getCurFunction();
-  assert(ScopeInfo->CoroutineParameterMoves.empty() &&
- "Should not build parameter moves twice");
+  if (!ScopeInfo->CoroutineParameterMoves.empty())
+return false;
 
   for (auto *PD : FD->parameters()) {
 if (PD->getType()->isDependentType())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JunMa via Phabricator via cfe-commits]

junparser added a comment.

In D69022#1756138 , @modocache wrote:

> LGTM, thanks! Please let me know if you'd like me to commit this change.


commit it please, thanks!


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

https://reviews.llvm.org/D69022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Brian Gesiak via Phabricator via cfe-commits]

modocache accepted this revision.
modocache added a comment.
This revision is now accepted and ready to land.

LGTM, thanks! Please let me know if you'd like me to commit this change.


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

https://reviews.llvm.org/D69022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JunMa via Phabricator via cfe-commits]

junparser added a comment.

In D69022#1755636 , @modocache wrote:

> 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 -U99 > 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.)


Thanks so much for reviewing the patch and giving the helpful advise.


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

https://reviews.llvm.org/D69022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JunMa via Phabricator via cfe-commits]

junparser updated this revision to Diff 230581.
junparser added a comment.

update as @modocache's suggestion


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

https://reviews.llvm.org/D69022

Files:
  clang/lib/Sema/SemaCoroutine.cpp
  clang/test/SemaCXX/coroutines.cpp


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -87,6 +87,11 @@
   co_await a;
 }
 
+int no_promise_type_multiple_awaits(int) { // expected-error {{this function 
cannot be a coroutine: 'std::experimental::coroutine_traits' has no 
member named 'promise_type'}}
+  co_await a;
+  co_await a;
+}
+
 template <>
 struct std::experimental::coroutine_traits { typedef int 
promise_type; };
 double bad_promise_type(double) { // expected-error {{this function cannot be 
a coroutine: 'experimental::coroutine_traits::promise_type' 
(aka 'int') is not a class}}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -1527,8 +1527,8 @@
   auto *FD = cast(CurContext);
 
   auto *ScopeInfo = getCurFunction();
-  assert(ScopeInfo->CoroutineParameterMoves.empty() &&
- "Should not build parameter moves twice");
+  if (!ScopeInfo->CoroutineParameterMoves.empty())
+return false;
 
   for (auto *PD : FD->parameters()) {
 if (PD->getType()->isDependentType())


Index: clang/test/SemaCXX/coroutines.cpp
===
--- clang/test/SemaCXX/coroutines.cpp
+++ clang/test/SemaCXX/coroutines.cpp
@@ -87,6 +87,11 @@
   co_await a;
 }
 
+int no_promise_type_multiple_awaits(int) { // expected-error {{this function cannot be a coroutine: 'std::experimental::coroutine_traits' has no member named 'promise_type'}}
+  co_await a;
+  co_await a;
+}
+
 template <>
 struct std::experimental::coroutine_traits { typedef int promise_type; };
 double bad_promise_type(double) { // expected-error {{this function cannot be a coroutine: 'experimental::coroutine_traits::promise_type' (aka 'int') is not a class}}
Index: clang/lib/Sema/SemaCoroutine.cpp
===
--- clang/lib/Sema/SemaCoroutine.cpp
+++ clang/lib/Sema/SemaCoroutine.cpp
@@ -1527,8 +1527,8 @@
   auto *FD = cast(CurContext);
 
   auto *ScopeInfo = getCurFunction();
-  assert(ScopeInfo->CoroutineParameterMoves.empty() &&
- "Should not build parameter moves twice");
+  if (!ScopeInfo->CoroutineParameterMoves.empty())
+return false;
 
   for (auto *PD : FD->parameters()) {
 if (PD->getType()->isDependentType())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 -U99 > 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' 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JunMa via Phabricator via cfe-commits]

junparser added a subscriber: rjmccall.
junparser added a comment.

In D69022#1720645 , @rjmccall wrote:

> Despite generally knowing about coroutines and generally knowing about Clang, 
> I actually don't know the Clang coroutine code and can't review this properly.


thanks  anyway~


Repository:
  rC Clang

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

https://reviews.llvm.org/D69022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[John McCall via Phabricator via cfe-commits]

rjmccall resigned from this revision.
rjmccall added a comment.

Despite generally knowing about coroutines and generally knowing about Clang, I 
actually don't know the Clang coroutine code and can't review this properly.


Repository:
  rC Clang

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

https://reviews.llvm.org/D69022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JunMa via Phabricator via cfe-commits]

junparser added a comment.

gental ping~  @modocache  @GorNishanov


Repository:
  rC Clang

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

https://reviews.llvm.org/D69022



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[JunMa via Phabricator via cfe-commits]

junparser created this revision.
junparser added reviewers: modocache, GorNishanov.
Herald added subscribers: cfe-commits, EricWF.
Herald added a project: clang.

The assertion of CoroutineParameterMoves happens when build coroutine function 
with arguments  multiple time while fails to build promise type.

Fix: use return false instead.

Test Plan: check-clang


Repository:
  rC Clang

https://reviews.llvm.org/D69022

Files:
  lib/Sema/SemaCoroutine.cpp
  test/SemaCXX/coroutines.cpp


Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -87,6 +87,11 @@
   co_await a;
 }
 
+int no_promise_type_2(int a) { // expected-error {{this function cannot be a 
coroutine: 'std::experimental::coroutine_traits' has no member named 
'promise_type'}}
+  co_await a;
+  co_await a;
+}
+
 template <>
 struct std::experimental::coroutine_traits { typedef int 
promise_type; };
 double bad_promise_type(double) { // expected-error {{this function cannot be 
a coroutine: 'experimental::coroutine_traits::promise_type' 
(aka 'int') is not a class}}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -1526,8 +1526,8 @@
   auto *FD = cast(CurContext);
 
   auto *ScopeInfo = getCurFunction();
-  assert(ScopeInfo->CoroutineParameterMoves.empty() &&
- "Should not build parameter moves twice");
+  if (!ScopeInfo->CoroutineParameterMoves.empty())
+return false;
 
   for (auto *PD : FD->parameters()) {
 if (PD->getType()->isDependentType())


Index: test/SemaCXX/coroutines.cpp
===
--- test/SemaCXX/coroutines.cpp
+++ test/SemaCXX/coroutines.cpp
@@ -87,6 +87,11 @@
   co_await a;
 }
 
+int no_promise_type_2(int a) { // expected-error {{this function cannot be a coroutine: 'std::experimental::coroutine_traits' has no member named 'promise_type'}}
+  co_await a;
+  co_await a;
+}
+
 template <>
 struct std::experimental::coroutine_traits { typedef int promise_type; };
 double bad_promise_type(double) { // expected-error {{this function cannot be a coroutine: 'experimental::coroutine_traits::promise_type' (aka 'int') is not a class}}
Index: lib/Sema/SemaCoroutine.cpp
===
--- lib/Sema/SemaCoroutine.cpp
+++ lib/Sema/SemaCoroutine.cpp
@@ -1526,8 +1526,8 @@
   auto *FD = cast(CurContext);
 
   auto *ScopeInfo = getCurFunction();
-  assert(ScopeInfo->CoroutineParameterMoves.empty() &&
- "Should not build parameter moves twice");
+  if (!ScopeInfo->CoroutineParameterMoves.empty())
+return false;
 
   for (auto *PD : FD->parameters()) {
 if (PD->getType()->isDependentType())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits