klimek added inline comments.
================
Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:46-49
+enum class MatchDirection {
+ Ancestors,
+ Descendants
+};
----------------
loic-joly-sonarsource wrote:
> 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.
What I'm trying to say is:
Currently the only non-transitive matchers we memoize are hasChild and
hasParent, which ... seems weird - my operating hypothesis is that back in the
day I didn't realize that or I'd have changed it :)
Thus, it seems like a simpler patch is to not memoize if it's not a transitive
match.
Sam also had a good idea, which is to change the ASTMatchFinder API to instead
of the current:
matchesChildOf
matchesDescendantOf
matchesAncestorOf(..., MatchMode)
create different atoms:
matchesChildOf
matchesDescendantOfOrSelf
matchesParentOf
matchesAncestorOfOrSelf
then hasDescendant(m) would be implemented as hasChild(hasDescendantOrSelf(m))
and the direction would be part of the matcher already.
That as an aside, which I'd propose to not do in the current patch though :)
My proposal for the current patch is to not memoize if we're only matching a
single child or parent.
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
