MatzeB added inline comments.
================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:724-730 + Stmt *BodyStmt = S.getBody(); + CompoundStmt *Body = dyn_cast<CompoundStmt>(BodyStmt); + if (Body == nullptr) { + Body = + CompoundStmt::Create(getContext(), {BodyStmt}, FPOptionsOverride(), + SourceLocation(), SourceLocation()); + } ---------------- ChuanqiXu wrote: > Can we try to move the logic to `CoroutineStmtBuilder`? That makes me feel > better. And it will be helpful to add a comment to tell that we're handling > the case the function body is function-try-block. I'll add a detailed comment. But would you be fine leaving the statements here as-is? The logic only makes sense in the context of using the `Body` to create a `CXXTryStmt` below (it's really an effect of `CXXTryStmt` only accepting CompountStmt operands). ================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:724-730 + Stmt *BodyStmt = S.getBody(); + CompoundStmt *Body = dyn_cast<CompoundStmt>(BodyStmt); + if (Body == nullptr) { + Body = + CompoundStmt::Create(getContext(), {BodyStmt}, FPOptionsOverride(), + SourceLocation(), SourceLocation()); + } ---------------- ChuanqiXu wrote: > MatzeB wrote: > > ChuanqiXu wrote: > > > Can we try to move the logic to `CoroutineStmtBuilder`? That makes me > > > feel better. And it will be helpful to add a comment to tell that we're > > > handling the case the function body is function-try-block. > > I'll add a detailed comment. But would you be fine leaving the statements > > here as-is? The logic only makes sense in the context of using the `Body` > > to create a `CXXTryStmt` below (it's really an effect of `CXXTryStmt` only > > accepting CompountStmt operands). > It looks like you didn't address the comments. Would you like to address it? > I don't mind to address it later myself. Did you mean to create a new function named `CoroutineStmtBuilder` like I did now? ================ Comment at: clang/lib/CodeGen/CGCoroutine.cpp:726 + CompoundStmt *Body = dyn_cast<CompoundStmt>(BodyStmt); + if (Body == nullptr) { + Body = ---------------- ChuanqiXu wrote: > It reads better to specify the potential type for Body. I will mention a single-statement body as an example in the comment now. Putting an `assert` here feels unnecessary and may be in the way if in the future we ever allow other types of single-statement function bodies. ================ Comment at: clang/test/CodeGenCoroutines/coro-function-try-block.cpp:21-23 +// CHECK-LABEL: define{{.*}} void @_Z1fv( +// CHECK: call void @_ZNSt13suspend_never13await_suspendESt16coroutine_handleIvE( +// CHECK: call void @_ZN4task12promise_type11return_voidEv( ---------------- ChuanqiXu wrote: > I expect to see the nested try statements in the case. This was mostly meant as a test for "does not crash the compiler", so it is just checking some parts of the output without caring too much what they are specifically. I cannot test for nested try statements, because in llvm-ir those are already lowered to unwind tables and landing pad blocks, but there's neither a "try" statement nor an immediate nesting I could test for. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146758/new/ https://reviews.llvm.org/D146758 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits