And the updated patch itself.<div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 26, 2012 at 12:59 PM, Alexander Kornienko <span dir="ltr"><<a href="mailto:alexfh@google.com" target="_blank">alexfh@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="gmail_extra"><div class="gmail_quote"><div class="im">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>On Thu, Jul 26, 2012 at 5:06 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>> wrote:<br>
> On Wed, Jul 25, 2012 at 11:04 PM, Manuel Klimek <<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>> wrote:<br>
</div><div>>> 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><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 class="im">
<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><div>Extend, basically. </div><div class="im"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>
>> -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><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 class="im">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>>> + 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><div>Both features are strongly related. Is it a bad idea to add them simultaneously?</div><div class="im"><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><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<span class="HOEnZb"><font color="#888888"><br>
</font></span></blockquote></div><span class="HOEnZb"><font color="#888888"><br><br clear="all"><div><br></div>-- <br>
</font></span></div><span class="HOEnZb"><font color="#888888"><div class="gmail_extra">Best regards,</div><div class="gmail_extra">Alexander Kornienko</div>
</font></span></blockquote></div><br><br clear="all"><div><br></div>-- <br><div><div><font color="#666666"><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(213,15,37);border-right-color:rgb(213,15,37);border-bottom-color:rgb(213,15,37);border-left-color:rgb(213,15,37);padding-top:2px;margin-top:2px">Alexander Kornienko |</span><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(51,105,232);border-right-color:rgb(51,105,232);border-bottom-color:rgb(51,105,232);border-left-color:rgb(51,105,232);padding-top:2px;margin-top:2px"> Software Engineer |</span></font><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(0,153,57);border-right-color:rgb(0,153,57);border-bottom-color:rgb(0,153,57);border-left-color:rgb(0,153,57);padding-top:2px;margin-top:2px"><font color="#666666"> </font><a href="mailto:alexfh@google.com" style="color:rgb(17,85,204)" target="_blank">alexfh@google.com</a> |</span><span style="border-top-width:2px;border-right-width:0px;border-bottom-width:0px;border-left-width:0px;border-top-style:solid;border-right-style:solid;border-bottom-style:solid;border-left-style:solid;border-top-color:rgb(238,178,17);border-right-color:rgb(238,178,17);border-bottom-color:rgb(238,178,17);border-left-color:rgb(238,178,17);padding-top:2px;margin-top:2px"> <a value="+35315435283" style="color:rgb(17,85,204)">+49 151 221 77 957</a></span></div>
</div><div><font color="#666666"><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">Google Germany GmbH | </span><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">Dienerstr. 12 | </span><span style="background-color:rgb(255,255,255);font-family:Arial,Verdana,sans-serif">80331 München</span></font></div>
<br>
</div>