[PATCH] D81885: [Coroutines] Return false on error of buildSuspends

[Brian Gesiak via Phabricator via cfe-commits]

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

I don't have a preference as to whether `Sema::ActOnCoroutineBodyStart` returns `true` or `false` to indicate failure, although I think returning `true` for failures is a bit of a pre-existing convention, such as here, where `true` is returned for an invalid declaration: https://github.com/llvm/llvm-project/blob/84167a8d58e8af79625abcffdf2c860d556955e6/clang/lib/Sema/SemaDecl.cpp#L1572

However I don't think this change is a good one because it removes test cases for what should be a an "NFC" change -- that is, changing the invalid return value from `true` to `false` shouldn't impact the behavior of the compiler in a way that necessitates test cases to be removed.



================
Comment at: clang/test/SemaCXX/coroutines.cpp:106
 double bad_promise_type_2(int) { // expected-error {{no member named 'initial_suspend'}}
-  co_yield 0; // expected-error {{no member named 'yield_value' in 'std::experimental::coroutine_traits<double, int>::promise_type'}}
+  co_yield 0;
 }
----------------
This test ought not change -- we do expect an error here. `co_yield` can only be used if the promise type defines `yield_value`.


================
Comment at: clang/test/SemaCXX/coroutines.cpp:479
       co_await transform_awaitable{};
-      // expected-error at -1 {{no member named 'await_ready'}}
     }
----------------
Same as my comment above: this is a valuable diagnostic, why remove the test for it? Same goes for the rest of the test changes in this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81885