[Coroutines] Return false on error of buildSuspends

[lxfind]

Summary

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

[lxfind]

Adjust tests

[modocache]

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.

[lxfind]

Makes sense. I will abandon this change.

clang/lib/Sema/SemaCoroutine.cpp

if (Suspend.isInvalid()) {
       Diag(KWLoc, diag::note_declared_coroutine_here) << Keyword;
       return StmtError();
     }
     return cast<Stmt>(Suspend.get());
   };
 
   StmtResult InitSuspend = buildSuspends("initial_suspend");
   if (InitSuspend.isInvalid())
-    return true;
+    return false;
 
   StmtResult FinalSuspend = buildSuspends("final_suspend");
   if (FinalSuspend.isInvalid())
-    return true;
+    return false;
 
   ScopeInfo->setCoroutineSuspends(InitSuspend.get(), FinalSuspend.get());
 
   return true;
 }
 
 // Recursively walks up the scope hierarchy until either a 'catch' or a function
 // scope is found, whichever comes first.