[PATCH] D86964: [ASTMatchers] Avoid recursion in ancestor matching to save stack space.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:705
+  // Unlike matchesAnyAncestorOf there's no memoization: it doesn't save much.
+  bool matchesParentOf(const DynTypedNode &Node, const DynTypedMatcher &Matcher,
+                       BoundNodesTreeBuilder *Builder) {
----------------
this API makes sense, I'm wondering whether we should do it further (more aligned with `matchesChildOf` pattern):

- make `matchesParentOf` public;
- introduce a new `matchesParentOf` interface in ASTMatcherFinder, which calls this function here;
- then we can remove the special parent case in `MatchASTVisitor::matchesAncestorOf`, and the `AncestorMatchMode` enum is also not needed anymore;

I'm also ok with the current way.


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:738
+    // Memoization keys that must be updated with the result.
+    std::vector<MatchKey> Keys;
+    // When returning, update the memoization cache.
----------------
nit: I think we should explicitly describe (either in the comment or name) these keys are for the single-parent *chain*. It took me a while to understand what are these keys for?


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:740
+    // When returning, update the memoization cache.
+    auto Finish = [&](bool Result) {
+      for (const auto &Key : Keys) {
----------------
maybe

Finish => Memorize
Result => IsMatched


================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:807
+      // Memoization of newly visited nodes is not possible (but we still update
+      // results for the elements in the chain we found above).
       std::deque<DynTypedNode> Queue(Parents.begin(), Parents.end());
----------------
nit: assert(Parents.size() > 1).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86964