[cfe-commits] [PATCH] Introduces DynTypedMatcher as a new concept that replaces the UntypedBaseMatcher and TypedMatcher. Due to DynTypedNode the basic dynamically typed matcher interface can now be simplified.

[Daniel Jasper]

  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