On Mon, Mar 11, 2013 at 9:05 AM, Daniel Jasper <[email protected]> wrote:
> Did you reply in phabricator or just on this email? > Just mail. > > > 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 && memoizedMatchesAncestorOfRecur > sively(...))) > return true; > } > > And then just remove the second loop? > Now that I think about it, we probably really want bfs (I think the confusion came because in the beginning this was not actually getting multiple parents): hasAncestor returns the first matching node, and we want that to be the closest matching node, as usually you'll want to say "give me the innermost function definition I'm in". I think not using bfs thus is a bug. Thx for pointing it out. Cheers, /Manuel
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
