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

[Loïc Joly via Phabricator via cfe-commits]

loic-joly-sonarsource marked an inline comment as done.
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:
> > > 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.
> 
> 
I've applied the change you suggested


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80025/new/

https://reviews.llvm.org/D80025