[cfe-commits] r160265 - in /cfe/trunk: include/clang/Tooling/CommandLineClangTool.h lib/Tooling/CMakeLists.txt lib/Tooling/CommandLineClangTool.cpp test/CMakeLists.txt test/Tooling/clang-ast-dump.cpp tools/CMakeLists.txt tools/Makefile tools/clan

[Alexander Kornienko]

And the updated patch itself.


On Thu, Jul 26, 2012 at 12:59 PM, Alexander Kornienko <alexfh at google.com>wrote:

> On Thu, Jul 26, 2012 at 9:52 AM, Manuel Klimek <klimek at google.com> wrote:
>
>> On Thu, Jul 26, 2012 at 5:06 AM, Alexander Kornienko <alexfh at google.com>
>> wrote:
>> > On Wed, Jul 25, 2012 at 11:04 PM, Manuel Klimek <klimek at google.com>
>> wrote:
>> >> High-level design of the new feature:
>> >> Are we sure that "substring of the node" is the desired interface? Why
>> >> not regexp? (heck, I'd love a dynamic matcher expression :P).
>> >
>> > Substring is quite useful, obvious and easy to implement. Regexps are
>> > better, but this could be in one of the next iterations. Dynamic matcher
>> > expressions would be great except that this would greatly increase
>> skills
>> > threshold required to use this tool ;)
>>
>> Yea, I was not serious about the matcher expressions. And, in the case
>> of an extra tool, I'd have been fine with this. For Clang itself, I'm
>> less sure that we're as happy to go and change stuff around.
>>
> 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).
>
> Doug? Any comment on how happy we are with having a command line
>> interface we know we'll want to change later?
>>
> Extend, basically.
>
> >> -ASTConsumer *CreateASTPrinter(raw_ostream *OS);
>> >> +ASTConsumer *CreateASTPrinter(raw_ostream *OS, const std::string
>> >> &FilterString);
>> >>
>> >> Any reasons not to use StringRef?
>> >
>> > The reason is that performance issues are not going to appear in this
>> code,
>> > which is invoked once per program execution.
>>
>> My impression is that Clang has a "use StringRef unless you have a
>> good reason not to". With that in place, not using StringRef makes
>> readers of the code stop and think why you did it.
>>
> I didn't find this specific rule, but there's no reason not to believe
> your words yet ;) Changed interface to use StringRef.
>
>
>> >> +  return new ASTPrinter(out, false, false, FilterString);
>> >>
>> >> Here, and below. Please use /*Name=*/ for context-less constants.
>> >
>> > Reasonable. Done.
>> >
>> >>
>> >> Additionally, it looks like ListAll is actually never true. Isn't this
>> >> a special case of filtering, where all nodes match, anyway?
>> >> If we need ListAll, I'd also propose to use a extra RAV implementation
>> >> that uses VisitNamedDecl, as it doesn't have anything else in common
>> >> with the visitation and really distracts from the more complex
>> >> matching case.
>> >
>> > Ok, moved to a separate class. I think it's useful to have a
>> > "-ast-list-decls" option for finding out what nodes can be filtered.
>>
>> I'm not saying it's not useful, but it's adding a second feature in
>> this patch :)
>
> Both features are strongly related. Is it a bad idea to add them
> simultaneously?
>
> +      if (filterMatches(D)) {
>> +        Out.changeColor(llvm::raw_ostream::BLUE) <<
>> +            (Dump ? "Dumping " : "Printing ") << getName(D) << ":\n";
>> +        Out.resetColor();
>> +        if (Dump)
>> +          D->dump(Out);
>> +        else
>> +          D->print(Out, /*Indentation=*/0, /*PrintInstantiation=*/true);
>> +        return true;
>>
>> Please add a comment before the "return true" that explains that we
>> stop traversing child nodes in this case.
>>
> Done.
>
>
>>
>> Cheers,
>> /Manuel
>>
>
>
>
> --
> Best regards,
> Alexander Kornienko
>



-- 
Alexander Kornienko | Software Engineer | alexfh at google.com | +49 151 221
77 957
Google Germany GmbH | Dienerstr. 12 | 80331 München
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120726/b589683f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ast-dump.diff
Type: application/octet-stream
Size: 14127 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120726/b589683f/attachment.obj>