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@lists.llvm.org <mailto: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. 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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
