================
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.
+ ///
----------------
Daniel Jasper wrote:
> Triggers what? I presume it is the registered callbacks?
Done.
================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:146
@@ +145,3 @@
+ }
+ void match(const clang::ast_type_traits::DynTypedNode &Node,
+ ASTContext &Context);
----------------
Daniel Jasper wrote:
> Should this be in the public interface?
Yes, as with the parent map in ASTContext having a DynTypedNode is actually
something that happens...
================
Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:167
@@ -160,1 +166,3 @@
+/// \brief Returns the results of matching \c Matcher on \c Node.
+///
----------------
Daniel Jasper wrote:
> 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.
It is not. 'match' only matches the node. This is a strict superset of the
findAll use case, but an important one - without it, it's impossible to run a
matcher against a single node (for example inside a match callback), which is
very useful (and the reason I started on this patch).
================
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);
----------------
Daniel Jasper wrote:
> Are you intentionally reusing the name of the methods you deleted above?
>
> Could this be templated and then work with other node types?
I was, thinking that we might want to keep them around, but after some arguing
on IRC with you I think we should leave out all other convenience stuff apart
from the match() function, which provides significant value as it allows a
different type of control flow for the code using the interface without the
need for implementing a callback.
================
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) {
----------------
Daniel Jasper wrote:
> 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.
As mentioned, I deleted those methods now.
================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:3482
@@ +3481,3 @@
+public:
+ explicit VerifyMatchOnNode(StringRef Id,
+ const internal::Matcher<T> &InnerMatcher)
----------------
Daniel Jasper wrote:
> nit: no need for explicit ..
Done.
================
Comment at: unittests/ASTMatchers/ASTMatchersTest.cpp:3500
@@ +3499,3 @@
+TEST(MatchFinder, CanMatchSingleNodesRecursively) {
+ EXPECT_TRUE(matchAndVerifyResultTrue(
+ "class X { class Y {}; };", recordDecl(hasName("::X")).bind("X"),
----------------
Daniel Jasper wrote:
> 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).
Now that we're only testing the match() interface, this test already exists ;)
http://llvm-reviews.chandlerc.com/D359
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
