[PATCH] Re-design the convenience interfaces on MatchFinder.

[Daniel Jasper]

================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:137
@@ -136,2 +136,3 @@
 
-  /// \brief Finds all matches on the given \c Node.
+  /// \brief Triggers on all matches on the given \c Node.
+  ///
----------------
Triggers what? I presume it is the registered callbacks?

================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:146
@@ +145,3 @@
+  }
+  void match(const clang::ast_type_traits::DynTypedNode &Node,
+             ASTContext &Context);
----------------
Should this be in the public interface?

================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:167
@@ -160,1 +166,3 @@
 
+/// \brief Returns the results of matching \c Matcher on \c Node.
+///
----------------
How about: "Returns the set of bound nodes for each match of \c Matcher under \c Node."

The "under" is also used below, but I don't know whether it makes perfectly clear that the matcher is matched on Node and all of its descendants.

================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:184
@@ +183,3 @@
+template <typename T>
+SmallVector<const T *, 1> findAll(internal::BindableMatcher<T> Matcher,
+                                  const Stmt &Node, ASTContext &Context);
----------------
Are you intentionally reusing the name of the methods you deleted above?

Could this be templated and then work with other node types?

================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:222
@@ +221,3 @@
+  SmallVector<BoundNodes, 1> Nodes =
+      match(stmt(forEachDescendant(Matcher.bind(""))),
+            ast_type_traits::DynTypedNode::create(Node), Context);
----------------
This does not match on \c Node itself, right? I think it should (and it also did with the old findAll functions).

================
Comment at: lib/ASTMatchers/ASTMatchFinder.cpp:473
@@ +472,3 @@
+  void match(const ast_type_traits::DynTypedNode& Node) {
+    for (std::vector<std::pair<const internal::DynTypedMatcher*,
+                               MatchCallback*> >::const_iterator
----------------
This was just moved, right? Otherwise, I'd say "consider using a typedef" ;-)...

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:3482
@@ +3481,3 @@
+public:
+  explicit VerifyMatchOnNode(StringRef Id,
+                             const internal::Matcher<T> &InnerMatcher)
----------------
nit: no need for explicit ..

================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:3500
@@ +3499,3 @@
+TEST(MatchFinder, CanMatchSingleNodesRecursively) {
+  EXPECT_TRUE(matchAndVerifyResultTrue(
+      "class X { class Y {}; };", recordDecl(hasName("::X")).bind("X"),
----------------
Independent of whether you change the behavior or not, I'd like a test where the recursive match happens on the node itself. (Probably easiest to use a function declaration for that to not get confused with the generated class name thing).

================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:219
@@ +218,3 @@
+template <typename T>
+SmallVector<const T *, 1> findAll(internal::BindableMatcher<T> Matcher,
+                                  const Stmt &Node, ASTContext &Context) {
----------------
During the experiments I have conducted, it was really convenient to pass in a matcher as well as a string to which stuff is bound in that matcher. I am aware that it is a slightly worse interface, but it gives significantly more freedom. And I think nothing can fail horribly, if nothing gets bound, then nothing is returned.

So my suggestion is: Add a second method that additionally takes a string "BoundTo" and implement this function here by just calling the other one.


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