Page MenuHomePhabricator

[Coroutines] Return false on error of buildSuspends

Authored by lxfind on Jun 15 2020, 2:35 PM.



I believe we need to return false when buildSuspends failed, to indicate that ActOnCoroutineBodyStart failed.

Diff Detail

Event Timeline

lxfind created this revision.Jun 15 2020, 2:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2020, 2:35 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lxfind updated this revision to Diff 270929.Jun 15 2020, 5:46 PM

Adjust tests

modocache requested changes to this revision.Jun 18 2020, 9:18 AM

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:

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.


This test ought not change -- we do expect an error here. co_yield can only be used if the promise type defines yield_value.


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.

This revision now requires changes to proceed.Jun 18 2020, 9:18 AM
lxfind abandoned this revision.Jun 18 2020, 10:54 AM

Makes sense. I will abandon this change.