LGTM
================ Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:36 @@ -33,3 +35,3 @@ // identifying the matcher) and a pointer to the AST node. typedef std::pair<uint64_t, const void*> UntypedMatchInput; ---------------- Manuel Klimek wrote: > Daniel Jasper wrote: > > Manuel Klimek wrote: > > > Daniel Jasper wrote: > > > > Add a comment why we don't want memoization on QualTypes (e.g. might be > > > > incorrect together with the coming hasAncestor matchers or unnecessary > > > > as there are no big QualType hierarchies). > > > > > > > > Also, maybe add FIXME to add memoization to for TypeLocs, once we match > > > > those (TypeLocs form bigger trees and presumably memoization is worth > > > > it). Also then it might make sense to store DynTypedNodes instead of > > > > void*!? > > > Tried to clarify in the comments. > > > > > > Every TypeLoc has a QualType, so I don't see why the second argument > > > holds - we'll either want to add memoization for both TypeLoc and > > > QualType, or for neither. > > > > > > Using DynTypedNode in the key doesn't make sense at all - we'll never > > > want to get the original value back, we just need a one-way function that > > > is unique. > > Yes, exactly, every TypeLoc has a QualType, so it is sufficient to memoize > > the TypeLoc (memoizing the QualTypes, too, would at best lead to an > > improvement by a small constant factor). However, TypeLocs can form longer > > chains and we don't want to match whole subchains repeatedly for > > hasDescendant().. > > > > "We just need a one-way function that is unique".. This is sort of what > > DynTypedNode provides (it already knows that for Decls and Stmts the > > pointer is unique and for QualTypes it has to use the value). Would be good > > to store information like that in only one class (even though we will never > > need the retrieval-logic). On the other hand, if you disagree on the first > > point and say that we will only ever want to memoize Stmts and Decls, this > > is a mute point ;-). > Why can TypeLocs form longer chains than QualTypes? > > I also think the notion of a key for a node is sufficiently different from a > pointer to the node. I'd rather pull out a NodeKey class or something at one > point. For now, I'd rather keep it simple. I think that is just what TypeLocs do ;-). I still think there is a lot of similarity between what we wnat to store here and what DynTypedNode currently does. However, I am perfectly happy keeping this simple (and a void*) in this CL. http://llvm-reviews.chandlerc.com/D33 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
