[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 2:13 PM, Manuel Klimek <klimek at google.com> wrote:

> On Thu, Jul 26, 2012 at 12:59 PM, Alexander Kornienko <alexfh at google.com>
> wrote:
> > 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.
>
> Yep. I'm in principle fine, but want to get input from people who have
> more experience how quickly others rely on specific semantics of
> command line flags.
>
> >> >> -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?
>
> I think it will make the review easier if we don't mix up the feature
> design discussion about the list-decl feature with the code review of
> the filtering change, which Doug already said he's in favor of.
>
Sounds reasonable. Here's a separate diff for -ast-dump-filter option.


> Cheers,
> /Manuel
>
> >
> >> +      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
>



-- 
Best regards,
Alexander Kornienko
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120726/803f1413/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ast-dump-filter.diff
Type: application/octet-stream
Size: 10142 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120726/803f1413/attachment.obj>