aaron.ballman added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5503
+ FunctionDecl,
+ CoroutineBodyStmt),
internal::Matcher<Stmt>, InnerMatcher) {
----------------
ccotter wrote:
> aaron.ballman wrote:
> > ccotter wrote:
> > > aaron.ballman wrote:
> > > > I'm not certain it makes sense to me to add `CoroutineBodyStmt` to
> > > > `hasBody` -- in this case, it doesn't *have* a body, it *is* the body.
> > > With respect to `hasBody()`, my intent was to treat the CoroutineBodyStmt
> > > node as analogous to a FunctionDecl or WhileStmt. WhileStmts have
> > > information like the loop condition expression, as CoroutineBodyStmts
> > > contain the synthesized expressions such as the initial suspend Stmt.
> > > Both WhileStmts and CoroutineBodyStmts then have the `getBody()` methods,
> > > usually a CompoundStmt for WhileStmts and either a CompoundStmt or
> > > TryStmt for CoroutineBodyStmts.
> > >
> > > Ultimately, I wanted to be able to match the CoroutineBodyStmt's
> > > `function-body` (using the grammar) from the standard, e.g.,
> > > `coroutineBodyStmt(hasBody(compoundStmt().bind(...)))`. If there is a
> > > different approach you'd recommend that's in line with the AST matcher
> > > design strategy, I can use that instead.
> > What concerns me about the approach you have is that it doesn't model the
> > AST. A coroutine body statement is a special kind of function body, so it
> > does not itself *have* a body. So this is a bit like adding `CompoundStmt`
> > to the `hasBody` matcher in that regard.
> >
> > To me, the correct way to surface this would be to write the matcher:
> > `functionDecl(hasBody(coroutineBodyStmt()))` to get to the coroutine body,
> > and if you want to bind to the compound statement, I'd imagine
> > `functionDecl(hasBody(coroutineBodyStmt(has(compoundStmt().bind(...)))))`
> > would be the correct approach. My thinking there is that we'd use the
> > `has()` traversal matcher to go from the coroutine body to any of the extra
> > information it tracks (the compound statement, the promise,
> > allocate/deallocate, etc).
> >
> > Pinging @klimek and @sammccall for additional opinions.
> I think I see. With my proposal, the match would be
> `functionDecl(hasBody(coroutineBodyStmt(hasBody(stmt().bind(...)))))`. For
> completeness, your suggestion would need
> `functionDecl(hasBody(coroutineBodyStmt(has(stmt(anyOf(cxxTryStmt(),
> compoundStmt()).bind(...))))))`.
>
> I am a bit hung up though on two things, and clarifications on both would
> help solidify my understanding of the design philosophy of the matchers.
> First, since `CoroutineBodyStmt` has a `getBody()` method, I assumed it would
> make it eligible for the `hasBody` matcher, although perhaps I am making an
> incorrect assumption/generalization here?
>
> Second, the `has()` approach seems less accurate since we would be relying on
> the fact that the other children nodes of `CoroutineBodyStmt` (initial or
> final suspend point, etc) would never be a `CompoundStmt` or `CXXTryStmt`,
> else we might get unintentional matches. Or, one might miss the CXXTryStmt
> corner case.
>
> Follow up question - should a need arise (say, authoring many clang-tidy
> checks that need extensive coroutine matcher support on the initial suspend
> point etc), would we see the matchers supporting things like
> `coroutineBodyStmt(hasInitialSuspendPoint(...), hasFinalSuspendPoint(..))`,
> or rely on a combination of `has` approach / non-ASTMatcher logic (or locally
> defined ASTMatchers within the clang-tidy library)?
>
> It looks like this phab can be broken down into two changes - first, the
> addition of a `coroutineBodyStmt` matcher, and second, a `hasBody` traversal
> matcher for `coroutineBodyStmt`. Happy to separate these out depending on the
> direction of the discussion.
> I am a bit hung up though on two things, and clarifications on both would
> help solidify my understanding of the design philosophy of the matchers.
> First, since CoroutineBodyStmt has a getBody() method, I assumed it would
> make it eligible for the hasBody matcher, although perhaps I am making an
> incorrect assumption/generalization here?
As I understand the evolution here, we started out with `hasBody()` to match on
function declaration AST nodes with a body. It then evolved to include
do/while/for (and eventually range-based for) loops, which is a bit of an odd
decision (why not anything with a substatement, like `if` or `switch`?). Now
we're looking to add support for the coroutine body node, but that has a half
dozen+ different things we can traverse to, one of which is the compound
statement for the coroutine body. All of these cases are kind of different from
one another, so the design is a bit confusing (at least to me).
> Second, the has() approach seems less accurate since we would be relying on
> the fact that the other children nodes of CoroutineBodyStmt (initial or final
> suspend point, etc) would never be a CompoundStmt or CXXTryStmt, else we
> might get unintentional matches. Or, one might miss the CXXTryStmt corner
> case.
That's a fantastic point that I hadn't considered. We probably will want named
traversal methods to go from the coroutine body statement to the other kinds of
AST nodes it supports. And based on that, `hasBody()` is a quite reasonable
name because that's one of the other kinds of AST nodes.
> Follow up question - should a need arise (say, authoring many clang-tidy
> checks that need extensive coroutine matcher support on the initial suspend
> point etc), would we see the matchers supporting things like
> coroutineBodyStmt(hasInitialSuspendPoint(...), hasFinalSuspendPoint(..)), or
> rely on a combination of has approach / non-ASTMatcher logic (or locally
> defined ASTMatchers within the clang-tidy library)?
I'm leaning more towards introducing new traversal matchers. In terms of what
goes into ASTMatchers.h, we usually only add matchers once there are multiple
in-tree needs for the matcher, otherwise we recommend using a locally defined
matcher. ASTMatchers.h is quite expensive to compile due to all the template
shenanigans, and so we've resisted adding matchers unless they're "needed" to
try to keep compile times reasonable.
Hmmm, I'm coming around to reusing `hasBody()` like you've done here. I think
it's pretty weird to read:
`functionDecl(hasBody(coroutineBodyStmt(hasBody(stmt().bind(...)))` because of
the duplicated calls to `hasBody`, but I think it's more defensible now than I
was originally thinking. I'm still curious if other folks who care about AST
matchers have an opinion given that this is a bit trickier than usual.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140794/new/
https://reviews.llvm.org/D140794
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
