On Mon, Dec 10, 2018 at 4:13 PM Richard Smith via cfe-commits
<cfe-commits@lists.llvm.org> wrote:
>
> On Mon, 10 Dec 2018 at 12:56, Stephen Kelly via cfe-commits
> <cfe-commits@lists.llvm.org> wrote:
>>
>> Author: steveire
>> Date: Mon Dec 10 12:53:32 2018
>> New Revision: 348794
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=348794&view=rev
>> Log:
>> Change InitListExpr dump to label and pointer
>>
>> Summary: Don't add a child just for the label.
>>
>> Reviewers: aaron.ballman
>>
>> Subscribers: cfe-commits
>>
>> Differential Revision: https://reviews.llvm.org/D55495
>>
>> Modified:
>> cfe/trunk/lib/AST/ASTDumper.cpp
>> cfe/trunk/test/AST/ast-dump-stmt.cpp
>>
>> Modified: cfe/trunk/lib/AST/ASTDumper.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348794&r1=348793&r2=348794&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/AST/ASTDumper.cpp (original)
>> +++ cfe/trunk/lib/AST/ASTDumper.cpp Mon Dec 10 12:53:32 2018
>> @@ -1951,11 +1951,12 @@ void ASTDumper::VisitInitListExpr(const
>> OS << " field ";
>> NodeDumper.dumpBareDeclRef(Field);
>> }
>> +
>> if (auto *Filler = ILE->getArrayFiller()) {
>> - dumpChild([=] {
>> - OS << "array filler";
>> - dumpStmt(Filler);
>> - });
>> + OS << " array_filler";
>> + NodeDumper.dumpPointer(Filler);
>> +
>> + dumpStmt(Filler);
>> }
>> }
>>
>>
>> Modified: cfe/trunk/test/AST/ast-dump-stmt.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-stmt.cpp?rev=348794&r1=348793&r2=348794&view=diff
>> ==============================================================================
>> --- cfe/trunk/test/AST/ast-dump-stmt.cpp (original)
>> +++ cfe/trunk/test/AST/ast-dump-stmt.cpp Mon Dec 10 12:53:32 2018
>> @@ -90,9 +90,8 @@ void TestUnionInitList()
>> {
>> U us[3] = {1};
>> // CHECK: VarDecl {{.+}} <col:3, col:15> col:5 us 'U [3]' cinit
>> -// CHECK-NEXT: `-InitListExpr {{.+}} <col:13, col:15> 'U [3]'
>> -// CHECK-NEXT: |-array filler
>> -// CHECK-NEXT: | `-InitListExpr {{.+}} <col:15> 'U' field Field {{.+}}
>> 'i' 'int'
>> +// CHECK-NEXT: `-InitListExpr {{.+}} <col:13, col:15> 'U [3]' array_filler
>> 0x{{.+}}
>> +// CHECK-NEXT: |-InitListExpr {{.+}} <col:15> 'U' field Field {{.+}} 'i'
>> 'int'
>> // CHECK-NEXT: `-InitListExpr {{.+}} <col:14> 'U' field Field {{.+}} 'i'
>> 'int'
>> // CHECK-NEXT: `-IntegerLiteral {{.+}} <col:14> 'int' 1
>> }
>
>
> I'm pretty strongly against this particular change.
Thank you for speaking up while this is still fresh in everyone's minds!
> I think this makes this form of InitListExpr dump *vastly* harder to read.
> The incredibly-easy-to-miss "array_filler" marker on the outer InitListExpr
> completely changes the meaning of the rest of the dump.
I don't see how this changes the meaning of the dump. It establishes
the relationship between an InitListExpr and its optional array filler
node, just like the original form did.
> The old dump form was much more readable. If you want to save vertical space,
> you could consider making the "array filler" label a prefix for the child:
>
> // CHECK: VarDecl {{.+}} <col:3, col:15> col:5 us 'U [3]' cinit
> // CHECK-NEXT: `-InitListExpr {{.+}} <col:13, col:15> 'U [3]'
> // CHECK-NEXT: |-array filler: InitListExpr {{.+}} <col:15> 'U' field Field
> {{.+}} 'i' 'int'
> // CHECK-NEXT: `-InitListExpr {{.+}} <col:14> 'U' field Field {{.+}} 'i'
> 'int'
> // CHECK-NEXT: `-IntegerLiteral {{.+}} <col:14> 'int' 1
>
> ... but I think the approach of this patch is not acceptable: it harms the
> primary purpose of -ast-dump, which is to form a human-readable dump of the
> AST.
The whitespace concerns are annoying because it becomes harder to
navigate the tree structure over larger dumps, but is not a
deal-breaker for me. The one-off nature of how this was being dumped
is a much bigger concern, and the way it was originally being handled
makes the refactoring Stephen and I are working on more awkward. We
did consider adding a prefix label, but that's also novel in the AST
dump. The form that I approved is consistent with how we handle
dumping relationships in other places, which is why I felt it was a
good compromise (in fact, to me, this consistency improves readability
over entire dumps because there are less novel relationship markers).
Given that we're trying to add a bit more structure to the dump, I
think what would help us is a consistent rule for how to lay out the
dump when showing a relationship between a parent and its multiple
child AST nodes. If you're fine with this:
// CHECK: VarDecl {{.+}} <col:3, col:15> col:5 us 'U [3]' cinit
// CHECK-NEXT: `-InitListExpr {{.+}} <col:13, col:15> 'U [3]'
// CHECK-NEXT: |-array filler: InitListExpr {{.+}} <col:15> 'U'
field Field {{.+}} 'i' 'int'
// CHECK-NEXT: `-InitListExpr {{.+}} <col:14> 'U' field Field {{.+}} 'i' 'int'
// CHECK-NEXT: `-IntegerLiteral {{.+}} <col:14> 'int' 1
Are you also fine if we use it consistently when dumping multiple
child nodes, like the combiner and initializer in
https://reviews.llvm.org/D55395? What about for the LHS and RHS of a
binary operator?
~Aaron
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
