[PATCH] D54309: [AST] Allow limiting the scope of common AST traversals (getParents, RAV).

[Eric Liu via Phabricator via cfe-commits]

ioeric added a subscriber: thakis.
ioeric added a comment.

The new API and the refactoring look good to me. Just a nit and a question.



================
Comment at: lib/AST/ASTContext.cpp:10292
+  if (!Parents)
     // We always need to run over the whole translation unit, as
     // hasAncestor can escape any subtree.
----------------
is this comment outdated?


================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:676
     const auto &Parents = ActiveASTContext->getParents(Node);
-    assert(!Parents.empty() && "Found node that is not in the parent map.");
     if (Parents.size() == 1) {
----------------
A quick search for "Found node that is not in the parent map." in my fav. search engine found a few prior discussions about this. E.g. @klimek seemed to want to remove the assertion (http://clang-developers.42468.n3.nabble.com/Question-about-failed-assertion-ASTMatchFinder-cc-quot-Found-node-that-is-not-in-the-parent-map-quot-td4038612.html), while @thakis seemed to favor keeping the assertion (https://bugs.chromium.org/p/chromium/issues/detail?id=580749).

Maybe we could still assertion if the scope if TU?


Repository:
  rC Clang

https://reviews.llvm.org/D54309