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@lists.llvm.org> wrote:
On Mon, 10 Dec 2018 at 12:56, Stephen Kelly via cfe-commits
<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.
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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
