<div dir="ltr">On Wed, Nov 6, 2013 at 9:29 AM, Samuel Benzaquen <span dir="ltr"><<a href="mailto:sbenza@google.com" target="_blank">sbenza@google.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
================<br>
Comment at: clang-query/QueryParser.h:20<br>
@@ +19,3 @@<br>
+///<br>
+/// \return A reference to the parsed query object, which may be an<br>
<div class="im">+/// \c InvalidQuery if a parse error occurs.<br>
</div>----------------<br>
There should be a comment somewhere specifying the valid commands and their syntax.<br>
It could in the parser, or somewhere in ClangQuery.cpp (possibly printing it to the user with a --help option).<br>
<div class="im"><br>
================<br>
Comment at: clang-query/Query.cpp:55-65<br>
@@ +54,13 @@<br>
+    CollectBoundNodes Collect(Matches);<br>
+    if (Matcher.canConvertTo<Decl>()) {<br>
+      DeclarationMatcher M = Matcher.convertTo<Decl>();<br>
+      if (QS.BindRoot)<br>
+        M = decl(M).bind("root");<br>
+      Finder.addMatcher(decl(forEachDescendant(M)), &Collect);<br>
+    } else if (Matcher.canConvertTo<Stmt>()) {<br>
+      StatementMatcher M = Matcher.convertTo<Stmt>();<br>
+      if (QS.BindRoot)<br>
+        M = stmt(M).bind("root");<br>
+      Finder.addMatcher(decl(forEachDescendant(M)), &Collect);<br>
+    }<br>
+    Finder.match(*AST->getASTContext().getTranslationUnitDecl(),<br>
----------------<br>
</div><div class="im">Manuel Klimek wrote:<br>
> Yea, we really need addDynamicMatcher ...<br>
</div>If I remember correctly, I added one, then we decide to remove it again.<br>
This tool would be the first use case that would require it.<br></blockquote><div><br></div><div>Peter said that according to the review history it fell out of a patch somehow :) I definitely think now is the time to add it, though, anyway :D</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
================<br>
Comment at: clang-query/Query.cpp:58<br>
@@ +57,3 @@<br>
<div class="im">+      if (QS.BindRoot)<br>
+        M = decl(M).bind("root");<br>
+      Finder.addMatcher(decl(forEachDescendant(M)), &Collect);<br>
</div>----------------<br>
You can bind at the DynTypedMatcher level using DynTypedMatcher::tryBind(). No need to do it for every type.<br>
<br>
================<br>
Comment at: clang-query/Query.cpp:59<br>
@@ +58,3 @@<br>
<div class="im">+        M = decl(M).bind("root");<br>
+      Finder.addMatcher(decl(forEachDescendant(M)), &Collect);<br>
+    } else if (Matcher.canConvertTo<Stmt>()) {<br>
</div>----------------<br>
What is the purpose of forEachDescendant() here?<br>
Finder will recursively try to match the provided matchers already.<br>
<br>
Doing it this way will match the topmost node if something in it matches, instead of returning the actual node that matched.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D2098" target="_blank">http://llvm-reviews.chandlerc.com/D2098</a><br>
<br>
BRANCH<br>
  clangquery<br>
<br>
ARCANIST PROJECT<br>
  clang-tools-extra<br>
</div></div></blockquote></div><br></div></div>