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
