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

[Michael Diamond]

  Generally looks good.  A few comments:


================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:86
@@ -71,1 +85,3 @@
+  /// pointers and thus need to be stored by value.
+  llvm::AlignedCharArrayUnion<Decl*, Stmt*, QualType> Storage;
 };
----------------
I think it would be better to use boost::any here. Then you don't have to explicitly specify all the types. You will still be able to store the QualType by value, but you won't have to worry about it in the declaration of Storage.

================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:131
@@ -133,1 +130,3 @@
+  std::vector<std::pair<const internal::DynTypedMatcher*, MatchCallback*> >
+    Triggers;
 
----------------
I would prefer a clearer name, even if it is on the wordy side. Trigger doesn't really fit. Some possibilities:
MatcherMatchActionPair
MatcherMatchCallbackPair
MatcherAndActionPair

================
Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:375
@@ -346,3 +374,3 @@
 /// \brief IsBaseType<T>::value is true if T is a "base" type in the AST
 /// node class hierarchies (i.e. if T is Decl, Stmt, or QualType).
 template <typename T>
----------------
This comment is stale; might as well fix it while we're nearby. The comment doesn't include CXXCtorInitializer.

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:92
@@ -77,1 +91,3 @@
+    if (Tag == NT_Decl)
+      return dyn_cast<T>(*reinterpret_cast<Decl*const*>(Storage));
     return NULL;
----------------
Why the change to reinterpret_cast? It seems a fair bit messier. This would also be cleaner using boost::any.

================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:83
@@ +82,3 @@
+  /// 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
----------------
typo: "guaranteed be" -> "guaranteed to be"


http://llvm-reviews.chandlerc.com/D33