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

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall added inline comments.


================
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) {
----------------
hokein wrote:
> 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.
I'm happy with that change, but I'd rather not bundle it into this one.
The goal here is to fix weird production characteristics (rather than APIs or algorithm output) and those are slippery, so let's change one thing at a time.


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

Memoize and Memorize are different words, and memoize doesn't act on results but rather on functions. So I think this rename is confusing.

> Result => IsMatched
Done



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86964