[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]

First, I'd split out the removal part and just submit it (lgtm for
deleting the tool, we have a version for our internal customers for
now (/me rubs hands) so we're fine until this is polished and
integrated).

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

-ASTConsumer *CreateASTPrinter(raw_ostream *OS);
+ASTConsumer *CreateASTPrinter(raw_ostream *OS, const std::string
&FilterString);

Any reasons not to use StringRef?

+  return new ASTPrinter(out, false, false, FilterString);

Here, and below. Please use /*Name=*/ for context-less constants.
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.

-void Decl::dumpXML() const {}
 void Decl::dumpXML(raw_ostream &out) const {}

Why's the lower one staying in?

Cheers,
/Manuel


On Wed, Jul 25, 2012 at 5:53 PM, Alexander Kornienko <alexfh at google.com> wrote:
> Hi all,
>
> This is the first step of what we were talking about: I removed the separate
> clang-ast-dump and added -ast-dump-filter option to clang -cc1. This option
> works both with -ast-dump and -ast-print and filters AST Decl nodes to dump
> by a substring of a qualified name. I also changed the behavior of
> Decl::dump method to really dump the node and not just print it to
> llvm::errs().
>
> Please review the patch so I can go on with making Decl::dump() output more
> structured and informational.
>
>
> On Fri, Jul 20, 2012 at 5:48 PM, Douglas Gregor <dgregor at apple.com> wrote:
>>
>>
>> On Jul 19, 2012, at 10:00 AM, Alexander Kornienko <alexfh at google.com>
>> wrote:
>>
>>
>> On Tue, Jul 17, 2012 at 7:55 PM, Douglas Gregor <dgregor at apple.com> wrote:
>>>
>>> Okay, that's fair; but this could trivially be added as a parameter to
>>> the existing XML dumper. We don't need a whole separate, single-purpose tool
>>> for this.
>>> ...
>>> > Overall, a way to dump the AST is critical to writing clang tools,
>>> > especially while you're learning the AST. Being able to dump parts of
>>> > the AST of the source you want to work on makes it far easier to find
>>> > out why the AST looks the way it does at a certain place where your
>>> > refactoring doesn't work than trying to reproduce that with a minimal
>>> > source file that doesn't use any non-builtin includes. Many tool
>>> > writers are not hardcore C++ experts (the ones that are obviously
>>> > don't need the tool that much), and one of the goals for us is to make
>>> > writing C++ tools possible for normal people ;)
>>>
>>> The XML dump isn't really a great way to learn the AST. Our normal dumper
>>> is far better for that purpose, IMO, because it's more complete and produces
>>> less noise.
>>
>>
>> Basically, we don't need XML, we only need a format which is closer to an
>> actual AST structure than just a C++ listing. Both XML and normal dumper use
>> the same format for statements, which is nice. The problem with the normal
>> dumper is how it outputs declarations. It basically outputs fragments in
>> C++, which isn't much useful for those trying to figure out how AST looks
>> like for declarations.If we could change its output to that LISP-style
>> format used for statements, it would be much more helpful in our use-case.
>>
>> Are there reasons for the normal dumper to exist in its current state?
>>
>>
>> I think it's simply laziness.
>>
>> If there are no serious reasons, I could work on switching it's format.
>> Otherwise we could add another dump format for declarations (consistent with
>> the one used for statements), or replace XML with it.
>>
>> What do you think about this?
>>
>>
>> I think this is the right way to go. -ast-dump should dump declarations in
>> the same tree format used for statements. -ast-print will still persist for
>> those who want to see pretty-printed ASTs rather than the actual form of the
>> ASTs.
>>
>> I would also be glad to add an "-ast-dump-filter" option to clang, so we
>> could select AST declaration nodes to dump by a substring. This would remove
>> any need in a separate clang-ast-dump tool.
>>
>>
>> That would be great!
>>
>> - Doug
>>
>
>
>
> --
> Alexander Kornienko | Software Engineer | alexfh at google.com | +49 151 221 77
> 957
> Google Germany GmbH | Dienerstr. 12 | 80331 München
>