Did you reply in phabricator or just on this email?
On Mon, Mar 11, 2013 at 3:57 PM, Manuel Klimek <[email protected]> wrote: > On Mon, Mar 11, 2013 at 7:19 AM, Daniel Jasper <[email protected]> wrote: > >> >> >> ================ >> Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:425 >> @@ -423,3 +424,3 @@ >> private: >> - bool matchesAncestorOfRecursively( >> + bool memoizedMatchesAncestorOfRecursively( >> const ast_type_traits::DynTypedNode &Node, const DynTypedMatcher >> &Matcher, >> ---------------- >> I think the combination of these two methods is highly confusing. The >> name suggests that both do the same thing, just one with memoization and >> one without memoization, which they don't. Furthermore, the >> responsibilities are not clear at all. E.g. what happens if I call >> matchesAncestorOfRecursively() with the TUDecl? It will probably assert, as >> the check for that is only in memoizedMatchesAncestorOfRecursively().. >> > > Any constructive ideas on how to change the layout? > Reading them a few more times, I don't find them so bad anymore. I think some of my confusion actually stems from the BFS/DFS combination, so lets solve that first. Also, I'd probably put the TUDecl-check into the other method. Basically, I would think that matchesAncestorOfRecursively() should work if it calls itself recursively (without the memoized..) variant. Also, it seems a bit strange, to first match all direct parents (kind of >> BFS) and then recursively call matchesAncestorOfRecursively() on each one >> (kind of DFS). Is this intended? >> > > Yes. That doesn't mean it was a good idea though :) > Why not just do DFS? I.e.: for (ASTContext::ParentVector::const_iterator AncestorI = Parents.begin(), AncestorE = Parents.end(); AncestorI != AncestorE; ++AncestorI) { if (Matcher.matches(*AncestorI, this, Builder) || (MatchMode == ASTMatchFinder::AMM_ParentOnly && memoizedMatchesAncestorOfRecursively(...)) ) return true; } And then just remove the second loop?
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
