[PATCH] D140794: [ASTMatcher] Add coroutineBodyStmt matcher

[Chris Cotter via Phabricator via cfe-commits]

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