[cfe-commits] PATCH: added clang-ast-dump tool

[Manuel Klimek]

On Fri, Jul 13, 2012 at 3:45 PM, Alexander Kornienko <alexfh at google.com> wrote:
> I've moved out common behavior and data for all command-line tools to a
> separate class (CommandLineClangTool.cpp), added more help, added one
> integration test for clang-ast-dump, added links to the recently added
> tooling setup how to. This should address most of the issues. I've decided
> to put off large-scale documentation efforts for now and first see how it
> goes in practical sense.

+++ tools/clang/test/Tooling/clang-ast-dump.cpp (revision 0)
There's an easier way to use this with the FixedCompilationDatabase (-- -c...)

I think that test is pretty brittle. I'd try to test the things we
care about: CXXMethod.*theMethod and BinaryOperator.*+, and stuff like
that. We want to minimize the chance that layout changes break the
test.

+class CommandLineClangTool {
+  CompilationDatabase *Compilations;
+  llvm::cl::opt<std::string> BuildPath;
+  llvm::cl::list<std::string> SourcePaths;
+  llvm::cl::extrahelp MoreHelp;

I usually prefer to have the public part first, as that's what's
relevant to the user.
Also, please add at least a short class comment and a comment on
initialize and run (for example, it's important to mention that
initialize does command line argument parsing).

Also, any reason not to have the constructor take argc & argv, as the
function doesn't return errors anyway?

+  cl::extrahelp MoreHelp(MoreHelpText);

Any reasons not to put this as a static global?

+  delete Compilations;

Any reasons not to make this an OwningPtr?

+  Compilations = NULL;

What would that help in the destructor?

Cheers,
/Manuel



>
>
> On Thu, Jul 12, 2012 at 9:57 PM, Sean Silva <silvas at purdue.edu> wrote:
>>
>> +//===- examples/Tooling/ClangCheck.cpp - Clang check tool
>> -----------------===//
>>
>> The file header is still the "ClangCheck" one.
>>
>> +//  This file implements a clang-ast-dump tool that dumps specified parts
>> +//  of an AST of a number of translation units.
>>
>> This header comment is rather uninformative (as are many others in the
>> clang codebase, unfortunately).
>>
>> It gives me a bit of a taste of what the tool does, but leaves me
>> hanging. The parts that I think "leave me hanging" are:
>>
>> "dumps *specified* parts" (emphasis mine): how do I specify them? what
>> things are matchable? can I use a regex? can I use the new ASTMatcher
>> library (although IIRC the dynamic matchers are not merged yet, so the
>> answer here is no)?
>>
>> "of a number of translation units": how do I specify them? do I need
>> some kind of special setup? if so, how so I set it up?
>>
>> As a person interested in using this tool, and maybe hacking on it, I
>> would like to at least see:
>>
>> * A high level description of the tool. As in all writing, remember
>> your audience: they have already seen the filename, so they can
>> already guess that it is related to "dumping ASTs"; thus a "high level
>> description" is going to be one level lower than "it dumps ASTs".
>> * Expected use cases and why this tool is needed (e.g. comparison with
>> `clang -ast-dump`)
>> * Quickstart. If a nontrivial setup is required, provide pointers to
>> the relevant documentation.
>> * Future directions you envision. One can only provide useful insight
>> into future directions after grokking the tool; hence the original
>> author is usually the best person to enunciate them. This usually
>> provides insight into the context which brought the tool into
>> existence and gives future hackers leads on where to continue.
>>
>> Thanks for the cool tool,
>> --Sean Silva
>>
>> On Thu, Jul 12, 2012 at 10:27 AM, Alexander Kornienko <alexfh at google.com>
>> wrote:
>> > Hi,
>> >
>> > This patch adds the clang-ast-dump tool based on the Clang Tooling
>> > infrastructure. It can help users of AST matchers to explore and
>> > understand
>> > AST by selectively dumping it. This is a first version aimed at
>> > collecting
>> > feedback and feature requests.
>> >
>> > --
>> > Regards,
>> > Alexander
>> >
>> > _______________________________________________
>> > cfe-commits mailing list
>> > cfe-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >
>
>
>
>
> --
> Alexander Kornienko | Software Engineer | alexfh at google.com | +49 151 221 77
> 957
> Google Germany GmbH | Dienerstr. 12 | 80331 München
>