[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

[Alexander Kornienko]

On Thu, Jul 26, 2012 at 9:52 AM, Manuel Klimek <klimek at google.com> wrote:

> On Thu, Jul 26, 2012 at 5:06 AM, Alexander Kornienko <alexfh at google.com>
> wrote:
> > On Wed, Jul 25, 2012 at 11:04 PM, Manuel Klimek <klimek at google.com>
> wrote:
> >> 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 ;)
>
> Yea, I was not serious about the matcher expressions. And, in the case
> of an extra tool, I'd have been fine with this. For Clang itself, I'm
> less sure that we're as happy to go and change stuff around.
>
Matching against substring is mostly a specific case of regular expression
matching. So this change (if anyone needs it) can be fully backwards
compatible (if I'm not wrong, qualified names usually don't contain special
characters used in regular expressions).

Doug? Any comment on how happy we are with having a command line
> interface we know we'll want to change later?
>
Extend, basically.

>> -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.
>
> My impression is that Clang has a "use StringRef unless you have a
> good reason not to". With that in place, not using StringRef makes
> readers of the code stop and think why you did it.
>
I didn't find this specific rule, but there's no reason not to believe your
words yet ;) Changed interface to use StringRef.


> >> +  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.
>
> I'm not saying it's not useful, but it's adding a second feature in
> this patch :)

Both features are strongly related. Is it a bad idea to add them
simultaneously?

+      if (filterMatches(D)) {
> +        Out.changeColor(llvm::raw_ostream::BLUE) <<
> +            (Dump ? "Dumping " : "Printing ") << getName(D) << ":\n";
> +        Out.resetColor();
> +        if (Dump)
> +          D->dump(Out);
> +        else
> +          D->print(Out, /*Indentation=*/0, /*PrintInstantiation=*/true);
> +        return true;
>
> Please add a comment before the "return true" that explains that we
> stop traversing child nodes in this case.
>
Done.


>
> Cheers,
> /Manuel
>



-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120726/b32ca8d8/attachment.html>