Mostly nits and comment changes ..
================
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
----------------
Blank line after \brief unless this is intentional.
================
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.
----------------
.. kinds of AST nodes ..
================
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.
----------------
maybe: (like \c Stmt and \c Decl)
================
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
----------------
\c QualType here and \c DynTypedNode below
================
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;
----------------
I'd be consistent with getMemoizationData (spacing around * and using
Storage.buffer).
Actually, how about:
return dyn_cast<T>(getMemoizationData());
?
================
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;
----------------
same as above
================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:352
@@ -363,2 +351,3 @@
+ MatchCallback*> >::const_iterator
It = Triggers->begin(), End = Triggers->end();
It != End; ++It) {
----------------
As you are here anyway, you could rename this to use the standard I and E, but
up to you.
================
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;
----------------
Some more documentation (or a reference) to what a QualType actually is
(Type-pointer + 3Bits) might help understanding this easier.
================
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;
----------------
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*!?
================
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.");
----------------
Why couldn't you reach this with a DynTypedNode representing a QualType? If you
can, add test.
http://llvm-reviews.chandlerc.com/D33
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
