[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]

  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