<div class="gmail_extra"><div class="gmail_quote">On Wed, Jul 25, 2012 at 11:04 PM, Manuel Klimek <span dir="ltr"><<a href="mailto:klimek@google.com" target="_blank">klimek@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

First, I'd split out the removal part and just submit it (lgtm for<br>
deleting the tool, we have a version for our internal customers for<br>
now (/me rubs hands) so we're fine until this is polished and<br>
integrated).<br></blockquote><div>Removed clang-ast-dump tool as a separate commit (r<span style="font-family:arial,sans-serif;font-size:13px">160772).</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
High-level design of the new feature:<br>
Are we sure that "substring of the node" is the desired interface? Why<br>
not regexp? (heck, I'd love a dynamic matcher expression :P).<br></blockquote><div>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 ;)</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
-ASTConsumer *CreateASTPrinter(raw_ostream *OS);<br>
+ASTConsumer *CreateASTPrinter(raw_ostream *OS, const std::string<br>
&FilterString);<br>
<br>
Any reasons not to use StringRef?<br></blockquote><div>The reason is that performance issues are not going to appear in this code, which is invoked once per program execution.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

+  return new ASTPrinter(out, false, false, FilterString);<br>
<br>
Here, and below. Please use /*Name=*/ for context-less constants.<br></blockquote><div>Reasonable. Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Additionally, it looks like ListAll is actually never true. Isn't this<br>
a special case of filtering, where all nodes match, anyway?<br>
If we need ListAll, I'd also propose to use a extra RAV implementation<br>
that uses VisitNamedDecl, as it doesn't have anything else in common<br>
with the visitation and really distracts from the more complex<br>
matching case.<br></blockquote><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

-void Decl::dumpXML() const {}<br>
 void Decl::dumpXML(raw_ostream &out) const {}<br>
<br>
Why's the lower one staying in?<br></blockquote><div>I don't want to remove dumpXML until dump output contains all information that currently exists in dumpXML.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Cheers,<br>
/Manuel<br>
<div><div><br>
<br>
On Wed, Jul 25, 2012 at 5:53 PM, Alexander Kornienko <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>> wrote:<br>
> Hi all,<br>
><br>
> This is the first step of what we were talking about: I removed the separate<br>
> clang-ast-dump and added -ast-dump-filter option to clang -cc1. This option<br>
> works both with -ast-dump and -ast-print and filters AST Decl nodes to dump<br>
> by a substring of a qualified name. I also changed the behavior of<br>
> Decl::dump method to really dump the node and not just print it to<br>
> llvm::errs().<br>
><br>
> Please review the patch so I can go on with making Decl::dump() output more<br>
> structured and informational.<br>
><br>
><br>
> On Fri, Jul 20, 2012 at 5:48 PM, Douglas Gregor <<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>> wrote:<br>
>><br>
>><br>
>> On Jul 19, 2012, at 10:00 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com" target="_blank">alexfh@google.com</a>><br>
>> wrote:<br>
>><br>
>><br>
>> On Tue, Jul 17, 2012 at 7:55 PM, Douglas Gregor <<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>> wrote:<br>
>>><br>
>>> Okay, that's fair; but this could trivially be added as a parameter to<br>
>>> the existing XML dumper. We don't need a whole separate, single-purpose tool<br>
>>> for this.<br>
>>> ...<br>
>>> > Overall, a way to dump the AST is critical to writing clang tools,<br>
>>> > especially while you're learning the AST. Being able to dump parts of<br>
>>> > the AST of the source you want to work on makes it far easier to find<br>
>>> > out why the AST looks the way it does at a certain place where your<br>
>>> > refactoring doesn't work than trying to reproduce that with a minimal<br>
>>> > source file that doesn't use any non-builtin includes. Many tool<br>
>>> > writers are not hardcore C++ experts (the ones that are obviously<br>
>>> > don't need the tool that much), and one of the goals for us is to make<br>
>>> > writing C++ tools possible for normal people ;)<br>
>>><br>
>>> The XML dump isn't really a great way to learn the AST. Our normal dumper<br>
>>> is far better for that purpose, IMO, because it's more complete and produces<br>
>>> less noise.<br>
>><br>
>><br>
>> Basically, we don't need XML, we only need a format which is closer to an<br>
>> actual AST structure than just a C++ listing. Both XML and normal dumper use<br>
>> the same format for statements, which is nice. The problem with the normal<br>
>> dumper is how it outputs declarations. It basically outputs fragments in<br>
>> C++, which isn't much useful for those trying to figure out how AST looks<br>
>> like for declarations.If we could change its output to that LISP-style<br>
>> format used for statements, it would be much more helpful in our use-case.<br>
>><br>
>> Are there reasons for the normal dumper to exist in its current state?<br>
>><br>
>><br>
>> I think it's simply laziness.<br>
>><br>
>> If there are no serious reasons, I could work on switching it's format.<br>
>> Otherwise we could add another dump format for declarations (consistent with<br>
>> the one used for statements), or replace XML with it.<br>
>><br>
>> What do you think about this?<br>
>><br>
>><br>
>> I think this is the right way to go. -ast-dump should dump declarations in<br>
>> the same tree format used for statements. -ast-print will still persist for<br>
>> those who want to see pretty-printed ASTs rather than the actual form of the<br>
>> ASTs.<br>
>><br>
>> I would also be glad to add an "-ast-dump-filter" option to clang, so we<br>
>> could select AST declaration nodes to dump by a substring. This would remove<br>
>> any need in a separate clang-ast-dump tool.<br>
>><br>
>><br>
>> That would be great!<br>
>><br>
>> - Doug<br>
>><br></div></div></blockquote></div>-- <br>
</div><div class="gmail_extra">Best regards,</div><div class="gmail_extra">Alexander Kornienko</div>