I believe we need to return false when buildSuspends failed, to indicate that ActOnCoroutineBodyStart failed.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
| Time | Test | |
|---|---|---|
| 130 ms | Clang.SemaCXX::Unknown Unit Message (" ") Script:
--
: 'RUN: at line 4'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/bin/clang -cc1 -internal-isystem /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/build/lib/clang/11.0.0/include -nostdsysteminc -std=c++14 -fcoroutines-ts -verify /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang/llvm-project/clang/test/SemaCXX/coroutines.cpp -fcxx-exceptions -fexceptions -Wunused-result
| |
| 190 ms | Clang.SemaCXX::Unknown Unit Message (" ") Script:
--
: 'RUN: at line 4'; c:\ws\prod\llvm-project\build\bin\clang.exe -cc1 -internal-isystem c:\ws\prod\llvm-project\build\lib\clang\11.0.0\include -nostdsysteminc -std=c++14 -fcoroutines-ts -verify C:\ws\prod\llvm-project\clang\test\SemaCXX\coroutines.cpp -fcxx-exceptions -fexceptions -Wunused-result
|
Event Timeline
Comment Actions
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.
| clang/test/SemaCXX/coroutines.cpp | ||
|---|---|---|
| 106 ↗ | (On Diff #270929) | This test ought not change -- we do expect an error here. co_yield can only be used if the promise type defines yield_value. |
| 479 ↗ | (On Diff #270929) | 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. |