================ 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: > Manuel Klimek wrote: > > Daniel Jasper wrote: > > > Triggers what? I presume it is the registered callbacks? > > Done. > ? Forgot to save after changing the comment :)
================ 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: > Manuel Klimek wrote: > > 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). > Ack. > > Might be easier to understand with a comment, though. I presume you need to > return a SmallVector to support matchers such as forEachDescendant? How about: > > "Returns the \c BoundNodes of all callback invocations when matching \p > Matcher on \p Node in \p Context."? > > How about choosing a name that reflects that this returns something? E.g. > "matchResults". Changed the comment, ptal. I don't find matchResults clearer ... I think match as free-standing function with this set of parameters is actually pretty clear. http://llvm-reviews.chandlerc.com/D359 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
