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