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

[Sean Silva]

  Looking pretty good!


================
Comment at: include/clang/ASTMatchers/ASTTypeTraits.h:92-132
@@ -71,23 +91,43 @@
 };
 template<typename T> struct DynTypedNode::BaseConverter<T,
-    typename llvm::enable_if<llvm::is_base_of<
-      Decl, typename llvm::remove_pointer<T>::type > >::type > {
-  static Decl *get(NodeTypeTag Tag, void *Node) {
-    if (Tag == NT_Decl) return static_cast<Decl*>(Node);
+    typename llvm::enable_if<llvm::is_base_of<Decl, T> >::type> {
+  static const T *get(NodeTypeTag Tag, const char Storage[]) {
+    if (Tag == NT_Decl)
+      return dyn_cast<T>(*reinterpret_cast<Decl*const*>(Storage));
     return NULL;
   }
-  static DynTypedNode create(const Decl *Node) {
-    return DynTypedNode(NT_Decl, Node);
+  static DynTypedNode create(const Decl &Node) {
+    DynTypedNode Result;
+    Result.Tag = NT_Decl;
+    new (Result.Storage.buffer) const Decl*(&Node);
+    return Result;
   }
 };
 template<typename T> struct DynTypedNode::BaseConverter<T,
-    typename llvm::enable_if<llvm::is_base_of<
-      Stmt, typename llvm::remove_pointer<T>::type > >::type > {
-  static Stmt *get(NodeTypeTag Tag, void *Node) {
-    if (Tag == NT_Stmt) return static_cast<Stmt*>(Node);
+    typename llvm::enable_if<llvm::is_base_of<Stmt, T> >::type> {
+  static const T *get(NodeTypeTag Tag, const char Storage[]) {
+    if (Tag == NT_Stmt)
+      return dyn_cast<T>(*reinterpret_cast<Stmt*const*>(Storage));
     return NULL;
   }
-  static DynTypedNode create(const Stmt *Node) {
-    return DynTypedNode(NT_Stmt, Node);
+  static DynTypedNode create(const Stmt &Node) {
+    DynTypedNode Result;
+    Result.Tag = NT_Stmt;
+    new (Result.Storage.buffer) const Stmt*(&Node);
+    return Result;
   }
 };
+template<> struct DynTypedNode::BaseConverter<QualType, void> {
+  static const QualType *get(NodeTypeTag Tag, const char Storage[]) {
+    if (Tag == NT_QualType)
+      return reinterpret_cast<const QualType*>(Storage);
+    return NULL;
+  }
+  static DynTypedNode create(const QualType &Node) {
+    DynTypedNode Result;
+    Result.Tag = NT_QualType;
+    new (Result.Storage.buffer) QualType(Node);
+    return Result;
+  }
+};
+// The only operation we allow on unsupported types is \c get.
----------------
It seems like all of these traits classes are basically the same boilerplate with NT_* used for the tag and the actual stored type used as the type of  the placement new and some reinterpret casting.

The only one that seems to differ between them is `get` which for Decl and Stmt has a dyn_cast, while QualType doesn't.

It would be nice if this duplication could be reduced, for example by having the traits class explicitly encode the constant (e.g. in an enum), and implementing as must as possible generically. I not, then at least a comment explaining the duplication situation would be good.

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:81-85
@@ -81,3 +80,7 @@
     reset();
-    traverse(Node);
+    if (const Decl *D = DynNode.get<Decl>())
+      traverse(*D);
+    else if (const Stmt *S = DynNode.get<Stmt>())
+      traverse(*S);
+    // FIXME: Add other base types after adding tests.
     return Matches;
----------------
A switch on the kind, with an "unreachable" default would allow clang to warn on incomplete coverage of the enum during future maintenance.

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:257-258
@@ -257,1 +256,4 @@
+    const UntypedMatchInput input(Matcher.getID(), Node.getMemoizationData());
+    assert(input.second &&
+           "Fix getMemoizationData once more types allow recursive matching.");
     std::pair<MemoizationMap::iterator, bool> InsertResult
----------------
Maybe put a `// FIXME` near this to keep things grep'able.

================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:98-100
@@ -97,1 +97,5 @@
 
+  /// \brief The \c MatchCallback*'s will be called every time the
+  /// \c DynTypedMatcher matches on the AST.
+  typedef std::pair<const internal::DynTypedMatcher*, MatchCallback*> Trigger;
+
----------------
The name "Trigger" here isn't really speaking to me (and the comment isn't helping much either).

After looking a bit more into what this is used for, I honestly don't know why the comment didn't come along as clear, since it describe it pretty well.

I think it has to do with the `'s` in `MatchCallback*'s`, which masks the clear 1-1 correspondence going on here. I think it would be better to phrase this as "A DynTypedMatcher and the MatchCallback to be invoked when it matches".

Sorry, idk why this comment threw me off, but for whatever reason I truly had to dive all the way into the source to get a clue about what this is even for.

Also, putting this right above the `Triggers` vector would increase locality.

================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:132-133
@@ -127,7 +131,4 @@
 private:
-  /// \brief The MatchCallback*'s will be called every time the
-  /// UntypedBaseMatcher matches on the AST.
-  std::vector< std::pair<
-    const internal::UntypedBaseMatcher*,
-    MatchCallback*> > Triggers;
+  /// \brief The triggers executed against the AST.
+  std::vector<Trigger> Triggers;
 
----------------
Since "trigger" is just a plain pair, it's not really clear at all what "executed against the AST means" for a trigger without some supporting documentation. Also as I mentioned above, putting the definition of `Trigger` nearby would help too.

(in general, a more self-documenting name than "trigger" would be good ("MatcherWithAction"?)).

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:353-355
@@ -367,4 +352,5 @@
+                             this, &Builder)) {
         BoundNodesTree BoundNodes = Builder.build();
         MatchVisitor Visitor(ActiveASTContext, It->second);
         BoundNodes.visitMatches(&Visitor);
       }
----------------
I know this isn't directly related to this patch, but this is really the nerve-center of the matching process, and deserves to be better explained (or cross-referenced to where it is explained). 


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