[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

[Manuel Klimek]

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.

Doug? Any comment on how happy we are with having a command line
interface we know we'll want to change later?

>> -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.

>> +  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 :)

+      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.

Cheers,
/Manuel