r348794 - Change InitListExpr dump to label and pointer

[Richard Smith via cfe-commits]

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. 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/670b54f5/attachment.html>