================
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:
> > 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
;-).
================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:96
@@ -77,1 +95,3 @@
+ if (Tag == NT_Decl)
+ return dyn_cast<T>(*reinterpret_cast<Decl*const*>(Storage));
return NULL;
----------------
Manuel Klimek wrote:
> Daniel Jasper wrote:
> > I'd be consistent with getMemoizationData (spacing around * and using
> > Storage.buffer).
> >
> > Actually, how about:
> > return dyn_cast<T>(getMemoizationData());
> > ?
> I dislike using getMemoizationData here. The other way around makes sense.
>
> Storage.buffer is different, because we get 'Storage' as a const char[] in
> this method. Perhaps I should rename it to Buffer to be more consistent?
Yes, the other way around makes more sense. I definitely like having only one
reinterpret_cast :-).
'Storage' is fine, I just did not read carefully enough, sorry!
================
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;
----------------
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).
http://llvm-reviews.chandlerc.com/D33
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
