On Mon, Oct 8, 2012 at 3:10 AM, Alexander Kornienko
<[email protected]> wrote:
>
> Except a couple of things (see inline comments) this looks good to me.
I'll fix all those.
> I'm not sure it makes sense to split this patch, but others may have
> different opinions.
Assuming it's not going to make it harder for you to review, I'd like
to split it up into at least these:
* a few small cleanups/fixes
* move parts of StmtDumper into ASTDumper (little or no change to behaviour)
* creation of DeclDumper (mostly new code)
Note that DeclDumper is still far from complete, so as it stands this
patch needs more work. I don't think it can be applied until it can at
least dump information equivalent to what is currently available from
-ast-dump. What I would like is agreement that this is the right
direction before I spend the effort to complete DeclDumper (and I
think we have the agreement now).
> FYI: I'm on vacation now, so I'll be very slow to respond in the next 3
> weeks.
>
> Doug, can you look at this?
>
>
> ================
> Comment at: lib/AST/ASTDumper.cpp:44
> @@ +43,3 @@
> +void ASTDumper::dumpAttrs(AttrVec &Attrs) {
> + for (AttrVec::const_iterator I=Attrs.begin(), E=Attrs.end(); I != E; ++I) {
> + IndentScope Indent(*this);
> ----------------
> Please add spaces around '='
>
> ================
> Comment at: lib/AST/ASTDumper.cpp:52
> @@ +51,3 @@
> + }
> + OS << "Attr" << " " << (const void*)A;
> + dumpSourceRange(A->getRange());
> ----------------
> Why are "Attr" and " " printed separately?
>
> ================
> Comment at: lib/AST/ASTDumper.cpp:32
> @@ +31,3 @@
> + NeedNewLine = true;
> + for (int i = 0, e = IndentLevel; i < e; ++i)
> + OS << " ";
> ----------------
> I think, it'd be better "OS.indent(IndentLevel * 2);"
> (http://llvm.org/docs/doxygen/html/classllvm_1_1raw__ostream.html#a8fdf5cdf041c8aded7e3308c1c3efacc)
>
> ================
> Comment at: test/Misc/ast-dump-decl.c:10
> @@ +9,3 @@
> +// CHECK: {{^}}(RecordDecl{{.*}}TestIndent{{[^()]*$}}
> +// CHECK-NEXT: {{^ }}(FieldDecl{{.*}}x{{[^()]*}})){{$}}
> +
> ----------------
> This is a matter of taste, probably, but I'd prefer to see one regex instead
> of 3 and more in a line, this also can make indentation a bit clearer. Just
> for comparison:
> {{^\(RecordDecl.*TestIndent[^()]*$}}
> {{^ \(FieldDecl.*x[^()]*\)\)$}}
>
>
> http://llvm-reviews.chandlerc.com/D52
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
