Committed as r160265.
On Mon, Jul 16, 2012 at 2:29 PM, Manuel Klimek <kli...@google.com> wrote:
> lgtm...
>
> we'll need to look how to make this work with the RefactoringTool next.
>
> On Mon, Jul 16, 2012 at 1:41 PM, Alexander Kornienko <ale...@google.com>
> wrote:
> >
> >
> >
> > On Fri, Jul 13, 2012 at 8:17 PM, Manuel Klimek <kli...@google.com>
> wrote:
> >>
> >> On Fri, Jul 13, 2012 at 3:45 PM, Alexander Kornienko <ale...@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 <sil...@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
> >> >> <ale...@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@cs.uiuc.edu
> >> >> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >> >> >
> >> >
> >> >
> >> >
> >> >
> >> > --
> >> > Alexander Kornienko | Software Engineer | ale...@google.com | +49 151
> >> > 221 77
> >> > 957
> >> > Google Germany GmbH | Dienerstr. 12 | 80331 München
> >> >
> >
> >
> >
> >
> > --
> > Alexander Kornienko | Software Engineer | ale...@google.com | +49 151
> 221 77
> > 957
> > Google Germany GmbH | Dienerstr. 12 | 80331 München
> >
>
--
Alexander Kornienko | Software Engineer | ale...@google.com | +49 151 221
77 957
Google Germany GmbH | Dienerstr. 12 | 80331 München
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
