r348794 - Change InitListExpr dump to label and pointer

[Stephen Kelly via cfe-commits]

I suggest continuing this discussion here: https://reviews.llvm.org/D55488

On 10/12/2018 21:42, Aaron Ballman wrote:
> On Mon, Dec 10, 2018 at 4:13 PM Richard Smith via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
>> On Mon, 10 Dec 2018 at 12:56, Stephen Kelly via cfe-commits <cfe-commits at 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