================
Comment at: include/clang/ASTMatchers/ASTMatchers.h:67
@@ -66,3 +66,3 @@
public:
/// \brief Returns the AST node bound to \c ID.
/// Returns NULL if there was no node bound to \c ID or if there is a node
but
----------------
Daniel Jasper wrote:
> Blank line after \brief unless this is intentional.
Done.
================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:28
@@ +27,3 @@
+/// Stores an AST node in a type safe way. This allows writing code that
+/// works with different kinds AST nodes, despite the fact that they don't
+/// have a common base class.
----------------
Daniel Jasper wrote:
> .. kinds of AST nodes ..
Done.
================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:51
@@ +50,3 @@
+ /// For types that have identity via their pointer in the AST
+ /// (like Stmt and Decl) the returned pointer points to the
+ /// referenced AST node.
----------------
Daniel Jasper wrote:
> maybe: (like \c Stmt and \c Decl)
Done.
================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:53
@@ +52,3 @@
+ /// referenced AST node.
+ /// For other types (like QualType) the value is stored directly
+ /// in the DynTypedNode, and the returned pointer points at
----------------
Daniel Jasper wrote:
> \c QualType here and \c DynTypedNode below
Done.
================
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:
> 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.
================
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;
----------------
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?
================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:110
@@ -88,1 +109,3 @@
+ if (Tag == NT_Stmt)
+ return dyn_cast<T>(*reinterpret_cast<Stmt*const*>(Storage));
return NULL;
----------------
Daniel Jasper wrote:
> same as above
Same answer :P
================
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:
> 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.
================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:260
@@ +259,3 @@
+ const UntypedMatchInput input(Matcher.getID(), Node.getMemoizationData());
+ assert(input.second &&
+ "Fix getMemoizationData once more types allow recursive matching.");
----------------
Daniel Jasper wrote:
> Why couldn't you reach this with a DynTypedNode representing a QualType? If
> you can, add test.
Because matchesDescendantOf (ASTMatchersInternal:444) only works for Decl and
Stmt currently...
================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:352
@@ -363,2 +351,3 @@
+ MatchCallback*> >::const_iterator
It = Triggers->begin(), End = Triggers->end();
It != End; ++It) {
----------------
Daniel Jasper wrote:
> As you are here anyway, you could rename this to use the standard I and E,
> but up to you.
Done.
http://llvm-reviews.chandlerc.com/D33
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
