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

[Manuel Klimek]

================
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;
 
----------------
Sean Silva wrote:
> 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"?)).
Yea, I didn't really like the name either, and the more I think about it, the worse "Trigger" sounds.

How about MatchAction?

================
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.
----------------
Sean Silva wrote:
> 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.
The problem is that the QualType one is sufficiently different (using the storage for storage of the actual object).

The other two are similar enough, but on the other hand I'm not sure whether pulling out yet another abstraction is a good idea already.

================
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;
----------------
Sean Silva wrote:
> A switch on the kind, with an "unreachable" default would allow clang to warn on incomplete coverage of the enum during future maintenance.
I actually don't want to expose the kind enum. Perhaps I'm a little too cautious, but I like my interfaces as small as possible.

I'm more thinking about having something built into RAV that would make sure all types are handled, but that would make RAV depend on the DynTypedNodes. I guess we'll see how it goes...


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