================ Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:89 @@ +88,3 @@ + /// Note that we store the QualType by value as we only get + /// it by-value from the AST. + llvm::AlignedCharArrayUnion<Decl*, Stmt*, QualType> Storage; ---------------- Daniel Jasper wrote: > Manuel Klimek wrote: > > Daniel Jasper wrote: > > > Some more documentation (or a reference) to what a QualType actually is > > > (Type-pointer + 3Bits) might help understanding this easier. > > I don't think we currently rely on that - for this implementation, all you > > need to know is that QualType doesn't have pointer identity and thus we > > need to store it by-value. > .. which I would find easier to understand than what the comment currently > says. > > How about: > > /// Note that we can store \c Decls and \c Stmts by pointer as they are > /// guaranteed be unique pointers pointing to dedicated storage in the > /// AST. \c QualTypes on the other hand do not have storage or unique > /// pointers and thus need to be stored by value. > /// Further info: <reference to QualType> > > (with the "Further info" completely optional). Done.
================ 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; ---------------- 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. http://llvm-reviews.chandlerc.com/D33 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
