<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jul 26, 2012 at 2:13 PM, 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 12:59 PM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>> wrote:<br>
> On Thu, Jul 26, 2012 at 9:52 AM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>> wrote:<br>
>><br>
>> On Thu, Jul 26, 2012 at 5:06 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>><br>
>> wrote:<br>
>> > On Wed, Jul 25, 2012 at 11:04 PM, Manuel Klimek <<a href="mailto:klimek@google.com">klimek@google.com</a>><br>
>> > wrote:<br>
>> >> 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<br>
>> > skills<br>
>> > threshold required to use this tool ;)<br>
>><br>
>> 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>
><br>
> Matching against substring is mostly a specific case of regular expression<br>
> matching. So this change (if anyone needs it) can be fully backwards<br>
> compatible (if I'm not wrong, qualified names usually don't contain special<br>
> characters used in regular expressions).<br>
><br>
>> Doug? Any comment on how happy we are with having a command line<br>
>> interface we know we'll want to change later?<br>
><br>
> Extend, basically.<br>
<br>
</div>Yep. I'm in principle fine, but want to get input from people who have<br>
more experience how quickly others rely on specific semantics of<br>
command line flags.<br>
<div><div class="h5"><br>
>> >> -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<br>
>> > code,<br>
>> > which is invoked once per program execution.<br>
>><br>
>> 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>
><br>
> I didn't find this specific rule, but there's no reason not to believe your<br>
> words yet ;) Changed interface to use StringRef.<br>
><br>
>><br>
>> >> + 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>
>> I'm not saying it's not useful, but it's adding a second feature in<br>
>> this patch :)<br>
><br>
> Both features are strongly related. Is it a bad idea to add them<br>
> simultaneously?<br>
<br>
</div></div>I think it will make the review easier if we don't mix up the feature<br>
design discussion about the list-decl feature with the code review of<br>
the filtering change, which Doug already said he's in favor of.<br></blockquote><div>Sounds reasonable. Here's a separate diff for -ast-dump-filter option.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Cheers,<br>
/Manuel<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
>> + 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>
><br>
> Done.<br>
><br>
>><br>
>><br>
>> Cheers,<br>
>> /Manuel<br>
><br>
><br>
><br>
><br>
> --<br>
> Best regards,<br>
> Alexander Kornienko<br>
</div></div></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>