loic-joly-sonarsource added inline comments.
================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:46-49
+enum class MatchDirection {
+ Ancestors,
+ Descendants
+};
----------------
klimek wrote:
> loic-joly-sonarsource wrote:
> > klimek wrote:
> > > Nice find! Why don't we need more states though?
> > > 1. wouldn't hasParent() followed by a hasAncestor() also trigger the
> > > memoization? (if so, we'd need transitive / non-transitive)
> > > 2. can we trigger a directional match and a non-directional
> > > (non-traversal, for example, explicit) match in the same memoization
> > > scope?
> > 1. Yes, I'm going to add it
> > 2. Sorry, I did not understand what you mean...
> >
> Re 2: nevermind, we don't memoize non-transitive matches (other than
> hasParent / hasChild), right?
> In that case, I'm wondering whether the simpler solution is to just not
> memoize hasParent / hasChild - wouldn't that be more in line with the rest of
> the memoization?
If I correctly understand what you mean, you think that memoizing recursive
searches (ancestors/descendants) might not be worth it, and that just memoizing
direct searches (parent, child) would be enough.
I really don't know if it's true or not, and I believe that getting this kind
of data requires a significant benchmarking effort (which code base, which
matchers...). And there might also be other performance-related concerns (for
instance, the value of `MaxMemoizationEntries`, the choice of `std::map` to
store the cache...),
In other words, I think this would go far beyond what this PR is trying to
achieve, which is only correcting a bug.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D80025/new/
https://reviews.llvm.org/D80025
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
