r348794 - Change InitListExpr dump to label and pointer

[Stephen Kelly via cfe-commits]

I'll revert it to allow for further discussion.

On 10/12/2018 21:12, Richard Smith wrote:
> On Mon, 10 Dec 2018 at 12:56, Stephen Kelly via cfe-commits 
> <cfe-commits at lists.llvm.org <mailto: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. 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.
>
> 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181210/6c3a39c2/attachment.html>