[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

Wed Jul 25 20:06:00 PDT 2012

On Wed, Jul 25, 2012 at 11:04 PM, Manuel Klimek <klimek at google.com> wrote:

> 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).
>
Removed clang-ast-dump tool as a separate commit (r160772).

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).
>
Substring is quite useful, obvious and easy to implement. Regexps are
better, but this could be in one of the next iterations. Dynamic matcher
expressions would be great except that this would greatly increase skills
threshold required to use this tool ;)

-ASTConsumer *CreateASTPrinter(raw_ostream *OS);
> +ASTConsumer *CreateASTPrinter(raw_ostream *OS, const std::string
> &FilterString);
>
> Any reasons not to use StringRef?
>
The reason is that performance issues are not going to appear in this code,
which is invoked once per program execution.

+  return new ASTPrinter(out, false, false, FilterString);
>
> Here, and below. Please use /*Name=*/ for context-less constants.
>
Reasonable. Done.

> 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.
>
Ok, moved to a separate class. I think it's useful to have a
"-ast-list-decls" option for finding out what nodes can be filtered.

-void Decl::dumpXML() const {}
>  void Decl::dumpXML(raw_ostream &out) const {}
>
> Why's the lower one staying in?
>
I don't want to remove dumpXML until dump output contains all information
that currently exists in dumpXML.

> 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
> >>
>
-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120726/91c1b4ff/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ast-dump.diff
Type: application/octet-stream
Size: 13708 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120726/91c1b4ff/attachment.obj>