[PATCH] D59076: [coroutines][PR40978] Emit error for co_yield within catch block

[Brian Gesiak via Phabricator via cfe-commits]

modocache added inline comments.


================
Comment at: lib/Sema/SemaCoroutine.cpp:197-200
-  if (S.isUnevaluatedContext()) {
-    S.Diag(Loc, diag::err_coroutine_unevaluated_context) << Keyword;
-    return false;
-  }
----------------
lewissbaker wrote:
> Does removing this check now mean that we're not checking that `co_return` statements don't appear in unevaluated contexts?
> 
> Or is that already handled elsewhere by the fact that `co_return` statements are not expressions and are therefore detected earlier as a grammar violation when parsing `sizeof()` expression?
That's right! If one were to attempt to use a `co_return` within a subexpression of `sizeof`, they'd see `error: expected expression` before they'd ever have seen this error message. So I believe there's no need to perform this check for `co_return`, and I believe that's why the revisions to the standard included in the Coroutines TS don't make any special mention of disallowing `co_return` in those contexts.


================
Comment at: lib/Sema/SemaCoroutine.cpp:675
+  // Second emphasis of [expr.await]p2: must be outside of an exception handler.
+  if (S.getCurScope()->getFlags() & Scope::CatchScope) {
+    S.Diag(Loc, diag::err_coroutine_within_handler) << Keyword;
----------------
lewissbaker wrote:
> modocache wrote:
> > EricWF wrote:
> > > We can still build a valid AST after encountering this error, no?
> > > 
> > > 
> > I believe so. Just to be clear: you'd like me to continue building the AST even after emitting this error diagnostic? My understanding is that most of this file bails soon after any error is encountered (correct me if that's wrong). I'm happy to change that, but I wonder if it'd be better to do that in a separate diff...?
> Do the scope flags reset when a lambda occurs within a catch-block?
> 
> eg. The following should still be valid.
> ```
> try { ... }
> catch (...) {
>   []() -> task { co_await foo(); }();
> }
> ```
I believe they're reset, the example you posted compiles fine with this patch. I'll add a test case specifically for this and confirm exactly where the scope flags are reset or ignored in the lambda case.


================
Comment at: test/SemaCXX/coroutines.cpp:299-311
   // FIXME: The spec says this is ill-formed.
   void operator=(CtorDtor&) {
     co_yield 0; // expected-error {{'co_yield' cannot be used in a copy assignment operator}}
   }
   void operator=(CtorDtor const &) {
     co_yield 0; // expected-error {{'co_yield' cannot be used in a copy assignment operator}}
   }
----------------
lewissbaker wrote:
> Not related to this diff, but...
> 
> I don't think that these should be ill-formed.
> 
> According to N4775 there are only exclusions added for [class.ctor] and [class.dtor].
> I can't see anything in the spec that says that assignment special member functions cannot be coroutines.
That's a great point. Could you create a Bugzilla for this work? And maybe we can get @GorNishanov's opinion?


Repository:
  rC Clang

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

https://reviews.llvm.org/D59076