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