ccotter added inline comments.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5503
+ FunctionDecl,
+ CoroutineBodyStmt),
internal::Matcher<Stmt>, InnerMatcher) {
----------------
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.
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
