[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 Tue, Jul 17, 2012 at 7:55 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On Jul 16, 2012, at 11:56 PM, Manuel Klimek <klimek at google.com> wrote:
>
>> On Tue, Jul 17, 2012 at 7:22 AM, Douglas Gregor <dgregor at apple.com> wrote:
>>>
>>> On Jul 16, 2012, at 5:46 AM, Alexander Kornienko <alexfh at google.com> wrote:
>>>
>>>> Author: alexfh
>>>> Date: Mon Jul 16 07:46:48 2012
>>>> New Revision: 160265
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=160265&view=rev
>>>> Log:
>>>> The new clang-ast-dump tool for selective AST dumping. Moved common command-line tool stuff to CommandLineClangTool
>>>
>>> I don't think this belongs in the Clang repository because:
>>>
>>>        1) It's built on a *debugging* option, the XML dumper, which is neither actively maintained nor complete enough to build any tool upon
>>>        2) It provides no value for users of Clang
>>>        3) It provides very little value for developers of Clang tools, since it does little more than clang + grep
>>>        4) It fails nearly all of the criteria for a Clang extension outlined in http://clang.llvm.org/get_involved.html
>>>
>>> I'll be happy to hear opinions to the contrary (Manuel?), but barring some killer argument for this feature, I'd like it reverted.
>>
>> I'll provide an argument for why I think this belongs in clang
>> mainline, but it's also no sweat to pull it into the (hopefully soon
>> available) tooling repo.
>
> FWIW, I'd argue against putting this into the tooling repo as well, unless someone is actually going to turn the XML dumper into something that's complete enough, well-documented, well-structured, etc. and could actually form the basis of real tools. But it's currently just a small debugging aid.
>
>> There are 2 killer features for this vs. clang -ast-dump-xml:
>> - clang -ast-dump-xml + grep doesn't scale: outputting a whole AST in
>> text often produces > 20GB of output and takes a long, long time -
>> grepping just on the node names is really fast
>
> 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.

True. That still leaves the other points.

>> - clang -ast-dump-xml requires the user to first get a correct -cc1
>> invocation, which is non-trivial for people who just write tools
>
>         -fsyntax-only -Xclang -ast-dump-xml
>
> should do the trick.

You still need to get the whole command line, don't you? The
interesting part about a standalone tool is that you are just
somewhere in the source tree and type:
$ tool-name some_source.cpp <+ options>
and get the output you need. Really easy to explore.

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

Having recently learned the AST I tend to disagree... The normal dump
mostly doesn't tell me anything about declarations. The -ast-dump-xml,
while not really being XML, is actually really nice to use. As I said,
we're happy to make improvements to it, too.

>> One thing that obviously needs to be addressed is what to do in
>> release mode. Independently of where we put this tool, I think having
>> a supported (we're going to support it ;) version of an ASTDumpXML
>> frontend action that works in release mode is important to us, so we
>> can build a tool and release it to our internal customers by default
>> in a release build. My proposal would be to change the builtin clang
>> version to completely not depend on it in release mode, as Alex
>> pointed out. Open to other ideas, though.
>>
>> Please advice in how to proceed. As I said, migrating this into the
>> new repo is easy enough. Also, any changes you want to see to make it
>> more in line with the extension policy would be interesting for me, so
>> I can make better decisions on changes like this in the future (I
>> think most of the policy points are met, or are meant to be met (minus
>> us screwing up, but that's why we have reviews :), or not applicable).
>
>
> Personally, I'm against having this tool exist in its current form in either the Clang repository or the as-yet-nonexistent Clang tools repository, because it isn't useful enough and making it a top-level two promises too much. The fact that it is XML printing exacerbates the problem, because so many users see an XML printing option and think that's the best way to interact with the compiler. It sets very high expectations, because there so much existing tooling around XML, and the XML dumper falls far, far short of meeting those expectations. That's why it's a hidden debugging aid, and should remain that way.
>
> This isn't the first time we've dealt with this issue. I removed the previous XML printer from Clang for exactly the same reasons that I object to this one: either we should do XML right, or we shouldn't expose it out to users.

I was not aware that we want ast-dump-xml to be all XML (as it's a
mixed mode output, which actually is quite nice). If -ast-dump did
exactly what -ast-dump-xml does, I'd be also fine with an ast-dump
tool. We (internally) definitely need a tool that does what currently
is closest provided by -ast-dump-xml, and we have good feedback from
our users on using it, with the main concern being them having to
build debug clang binaries and it being hard to run over arbitrary
sources they have lying around.

Cheers,
/Manuel