[PATCH] D116328: [ast-matchers] Add hasSubstatement() traversal matcher for caseStmt(), defaultStmt(), labelStmt()

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood marked an inline comment as done.
LegalizeAdulthood added a comment.

In D116328#3221995 <https://reviews.llvm.org/D116328#3221995>, @aaron.ballman wrote:

>> Previously if you wanted to match the statement associated with a case, default, or labelled statement, you had to use hasDescendant.
>
> This isn't true. You can use `has()` to do direct substatemematching, and I'm wondering whether we need this new matcher at all. e.g., https://godbolt.org/z/K5sYP757r

Well, the whole point of this was that previous review comments were complaining that I was using an "expensive" matcher
instead of one that got right to the point.  (What's the difference between `has` and `hasDescendent` anyway?)

I looked at the implementation of "has" and it has a huge amount of machinery underneath it.  I drilled in as follows (my ToT hash is 773ab3c6655f4d2beec25bb3516b4d4fe2eea990 <https://reviews.llvm.org/rG773ab3c6655f4d2beec25bb3516b4d4fe2eea990>):
ASTMatchers.h:3391 declares `has` as an instance of `internal::ArgumentAdaptingMatcherFunc<internal::HasMatcher>`
`ArgumentAdaptingMatcherFun` appears just to be a factory mechanism for creating the appropriate matcher.
`HasMatcher` seems to do the actual matching
`HasMatcher<T, ChildT>::matches` delegates to `ASTMatchFinder::matchesChildOf(const T &Node, ...)`
`ASTMatchFinder::matchesChildOf(const T &Node, ...)` delegates to another overload of `matchesChildOf`(const DynTypedNode &Node, ...)` after asserting a static type relationship (ASTMatchersInternal.h:762)
`ASTMatchFinder::matchesChildOf(const DynTypedNode &Node, ...)` is a virtual function with one implementation in `MatchASTVisitor` (ASTMatchFinder.cpp:659)
`MatchASTVisitor::matchesChildOf` delegates to `memoizedMatchesRecursively` (ASTMatchFinder.cpp:664)
For non-memoizable nodes, `MatchASTVisitor::memoizedMatchesRecursively` delegates to `matchesRecursively` (ASTMatchFinder.cpp:599)

  `MatchASTVIsitor::matchesRecursively` instantiates a `ASTNodeNotSpelledInSourceScope` and a `MatchChildASTVisitor` upon which it calls `findMatch` (ASTMatchFinder.cpp:645)
  `MatchChildASTVisitor::findMatch` does a series of `DynNode.get<T>` checks and delegates to the appropriate overload of `MatchChildASTVisitor::traverse`, in our case it would be the one for `Stmt`
  `MatchChildASTVisitor::traverse<T>(const T &Node)` delegates to `MatchChildASTVisitor::match<T>(const T &Node)`
  `MatchChildASTVisitor::match<T>(const T &Node)` instantiates a `BoundNodesTreeBuilder` and calls `match` on the held `Matcher`.

Honestly, at this point, I become lost in manually tracing the code through "go to definition" in my IDE.  So I switched my unit test for `HasCaseSubstmt` to use `has` and drilled in through the debugger.
What I saw echoes the above, although I took the wrong branch in my manual analysis in `memoizedMatchesRecursively`, it goes down the memoization path.

However, the whole point of memoizing a result is because that result was expensive to compute and the memoization saves time on subsequent queries
because the result has been memorized.  In this case, it's a simple getter on `CaseStmt` to obtain the node against which you want to match, so I don't
see the memoization as having any particular benefit.  (The inner matcher to `hasSubstatement` may be expensive and could be memoized.)  For results
obtain by the visitor pattern, I can see why you'd want to memoize them as there is lots of code being executed to implement the Visitor pattern.



================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:2432
     dependentCoawaitExpr;
+
 /// Matches co_yield expressions.
----------------
aaron.ballman wrote:
> Spurious newline?
I didn't add this intentionally and I can remove it, but the general style in this file is to separate top-level decls by a blank line.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116328/new/

https://reviews.llvm.org/D116328