r348794 - Change InitListExpr dump to label and pointer

[Aaron Ballman via cfe-commits]

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