aaron.ballman added a comment. Aside from some changes to the test coverage, I think @hokein and I are in agreement on moving forward with the patch as-is (feel free to correct me if I'm wrong though!).
================ 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); } ---------------- hokein wrote: > 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. > > 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. Agreed, but it's also pretty weird to have a `CallExpr` AST node where no call expression exists in the source code. For example, when we do an -ast-print for the test case below, we get some fun behavior: ``` Chat f(int s) { co_yield __promise.yield_value(s); co_return s; co_await s; } ``` https://godbolt.org/z/KxzPoPz7T and this spills over into AST matchers that try to ignore code not spelled in source: https://godbolt.org/z/fKhjMEs5P (note match #4, for example). > 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. That's a pretty good idea -- having a semantic and syntactic form does seem like it would be a good approach in the future. 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
