<div class="gmail_extra"><div class="gmail_quote">On Thu, Jul 26, 2012 at 9:52 AM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">On Thu, Jul 26, 2012 at 5:06 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>> wrote:<br>
> On Wed, Jul 25, 2012 at 11:04 PM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
</div><div class="im">>> High-level design of the new feature:<br>
>> Are we sure that "substring of the node" is the desired interface? Why<br>
>> not regexp? (heck, I'd love a dynamic matcher expression :P).<br>
><br>
> Substring is quite useful, obvious and easy to implement. Regexps are<br>
> better, but this could be in one of the next iterations. Dynamic matcher<br>
> expressions would be great except that this would greatly increase skills<br>
> threshold required to use this tool ;)<br>
<br>
</div>Yea, I was not serious about the matcher expressions. And, in the case<br>
of an extra tool, I'd have been fine with this. For Clang itself, I'm<br>
less sure that we're as happy to go and change stuff around.<br></blockquote><div>Matching against substring is mostly a specific case of regular expression matching. So this change (if anyone needs it) can be fully backwards compatible (if I'm not wrong, qualified names usually don't contain special characters used in regular expressions).</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Doug? Any comment on how happy we are with having a command line<br>
interface we know we'll want to change later?<br></blockquote><div>Extend, basically. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">

>> -ASTConsumer *CreateASTPrinter(raw_ostream *OS);<br>
>> +ASTConsumer *CreateASTPrinter(raw_ostream *OS, const std::string<br>
>> &FilterString);<br>
>><br>
>> Any reasons not to use StringRef?<br>
><br>
> The reason is that performance issues are not going to appear in this code,<br>
> which is invoked once per program execution.<br>
<br>
</div>My impression is that Clang has a "use StringRef unless you have a<br>
good reason not to". With that in place, not using StringRef makes<br>
readers of the code stop and think why you did it.<br></blockquote><div>I didn't find this specific rule, but there's no reason not to believe your words yet ;) Changed interface to use StringRef.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">>> +  return new ASTPrinter(out, false, false, FilterString);<br>
>><br>
>> Here, and below. Please use /*Name=*/ for context-less constants.<br>
><br>
> Reasonable. Done.<br>
><br>
>><br>
>> Additionally, it looks like ListAll is actually never true. Isn't this<br>
>> a special case of filtering, where all nodes match, anyway?<br>
>> If we need ListAll, I'd also propose to use a extra RAV implementation<br>
>> that uses VisitNamedDecl, as it doesn't have anything else in common<br>
>> with the visitation and really distracts from the more complex<br>
>> matching case.<br>
><br>
> Ok, moved to a separate class. I think it's useful to have a<br>
> "-ast-list-decls" option for finding out what nodes can be filtered.<br>
<br>
</div>I'm not saying it's not useful, but it's adding a second feature in<br>
this patch :)</blockquote><div>Both features are strongly related. Is it a bad idea to add them simultaneously?</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

+      if (filterMatches(D)) {<br>
+        Out.changeColor(llvm::raw_ostream::BLUE) <<<br>
+            (Dump ? "Dumping " : "Printing ") << getName(D) << ":\n";<br>
+        Out.resetColor();<br>
+        if (Dump)<br>
+          D->dump(Out);<br>
+        else<br>
+          D->print(Out, /*Indentation=*/0, /*PrintInstantiation=*/true);<br>
+        return true;<br>
<br>
Please add a comment before the "return true" that explains that we<br>
stop traversing child nodes in this case.<br></blockquote><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
/Manuel<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>
</div><div class="gmail_extra">Best regards,</div><div class="gmail_extra">Alexander Kornienko</div>