hokein added inline comments.
================ Comment at: clang/lib/Sema/SemaCoroutine.cpp:322 + auto EndLoc = Args.empty() ? Loc : Args.back()->getEndLoc(); + return S.BuildCallExpr(nullptr, Result.get(), Loc, Args, EndLoc, nullptr); } ---------------- aaron.ballman wrote: > hokein wrote: > > aaron.ballman wrote: > > > hokein wrote: > > > > Thanks for the fast fix. > > > > > > > > Reading the source code here, I'm not sure this is a correct fix (and > > > > put the heuristic here). I assume the `Loc` points the the `co_yield` > > > > token here, passing the `Loc` to the `LParenLoc` and `RParenLoc` > > > > parameters seems wrong to me, there is no `(` and `)` written in the > > > > source code here (because the member call expression is an implicit > > > > generated node in the clang AST), we should pass an invalid source > > > > location (`SourceLocation()`). > > > > > > > > The `CallExpr::getEndLoc()` has implemented similar > > > > [heuristic](https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/Expr.cpp#L1662-L1664) > > > > to handle the case where the RParenLoc is invalid. > > > > passing the Loc to the LParenLoc and RParenLoc parameters seems wrong > > > > to me > > > > > > FWIW, I thought so as well, but I thought we have the same behavior for > > > `CoreturnExpr` and `CoawaitExpr`. These statements are not really call > > > expressions as far as the AST is concerned, so it's a bit hard to say > > > whether there is a right or wrong answer for where the l- and r-paren > > > locations are for the call. > > Agree, the coroutine is a hard case. I was concerned about the > > `MemberCallExpr::getRParenLoc()` API, it returns a valid source location > > but the token it points to is not a `)`, this could lead to subtle bugs in > > tooling. > > > > since this change is an improvement, I'm fine with the current change. > > Agree, the coroutine is a hard case. I was concerned about the > > MemberCallExpr::getRParenLoc() API, it returns a valid source location but > > the token it points to is not a ), this could lead to subtle bugs in > > tooling. > > Yeah, that was actually my concern as well -- I'm glad we're both on the same > page. :-) > > I eventually convinced myself that folks are generally interested in the > right paren location because they want to know when the argument list is > complete, not because they specifically want the right paren token itself. > But the reason they want to do that is because they want to insert a > qualifier, a semi-colon, an attribute, etc (something that follows the > closing paren) and in all of those cases, they'll be surprised here. Perhaps > we should be marking this call expression as an implicit AST node so that > it's more clear to users "this wasn't from source, don't expect fix-its to be > a good idea?" > I eventually convinced myself that folks are generally interested in the > right paren location because they want to know when the argument list is > complete, not because they specifically want the right paren token itself. > But the reason they want to do that is because they want to insert a > qualifier, a semi-colon, an attribute, etc (something that follows the > closing paren) and in all of those cases, they'll be surprised here. Yes, exactly. > Perhaps we should be marking this call expression as an implicit AST node so > that it's more clear to users "this wasn't from source, don't expect fix-its > to be a good idea?" Possibly. My understanding of this expression is that it represents the written `operand` of the `co_yield` expression to some degree (see https://github.com/llvm/llvm-project/blob/main/clang/include/clang/AST/RecursiveASTVisitor.h#L2901). If we marked it implicit, then there is no "visible" expression in the `CoyieldExpr`, which means the operand will not be visited. Overall, it looks like the current AST node for `co_yield`, `co_await`, `co_await` expressions are mostly modeling the language semantics, thus they are complicated, I guess this is the reason why it feels hard to improve the support for the syntactic. Probably, we can borrow the idea from `InitListExpr`, there are two forms, one is for semantic, the other one is for syntactic, having these two split can make everything easier. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157296/new/ https://reviews.llvm.org/D157296 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
