[PATCH] D80025: [ASTMatcher] Correct memoization bug ignoring direction (descendants or ancestors)

[Manuel Klimek via Phabricator via cfe-commits]

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