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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
