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

[Alexander Kornienko]

On Fri, Jul 13, 2012 at 8:17 PM, Manuel Klimek <klimek at google.com> wrote:

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

Done, reduced to a reasonable minimum.


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


> Also, any reason not to have the constructor take argc & argv, as the
> function doesn't return errors anyway?
>
Yep, the reason is to allow user to add some help text after the common
help text added in CommandLineClangTool constructor and before parsing
command-line options.

+  cl::extrahelp MoreHelp(MoreHelpText);
>
> Any reasons not to put this as a static global?
>
Same as the previous one.


> +  delete Compilations;
>
> Any reasons not to make this an OwningPtr?
>
No serious reasons.

> +  Compilations = NULL;
>
> What would that help in the destructor?
>
 Idiom "delete p; p = NULL;", so a pointer is always valid or null. Anyway,
now it's OwningPtr.


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



-- 
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/20120716/6f1649d3/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-tools.diff
Type: application/octet-stream
Size: 19658 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120716/6f1649d3/attachment.obj>