[cfe-commits] [PATCH] Implement AST dumper for Decls

[Philip Craig]

On Mon, Oct 8, 2012 at 3:10 AM, Alexander Kornienko
<reviews at llvm-reviews.chandlerc.com> 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