<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>I'll revert it to allow for further discussion.<br>
</p>
<div class="moz-cite-prefix">On 10/12/2018 21:12, Richard Smith
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAOfiQqmxcWyC3LxQCe1XoQ307n3mr7p3F0sGYbuvNDPjs9KW8A@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr">On Mon, 10 Dec 2018 at 12:56, Stephen Kelly via
cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org"
moz-do-not-send="true">cfe-commits@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">Author:
steveire<br>
Date: Mon Dec 10 12:53:32 2018<br>
New Revision: 348794<br>
<br>
URL: <a
href="http://llvm.org/viewvc/llvm-project?rev=348794&view=rev"
rel="noreferrer" target="_blank" moz-do-not-send="true">http://llvm.org/viewvc/llvm-project?rev=348794&view=rev</a><br>
Log:<br>
Change InitListExpr dump to label and pointer<br>
<br>
Summary: Don't add a child just for the label.<br>
<br>
Reviewers: aaron.ballman<br>
<br>
Subscribers: cfe-commits<br>
<br>
Differential Revision: <a
href="https://reviews.llvm.org/D55495" rel="noreferrer"
target="_blank" moz-do-not-send="true">https://reviews.llvm.org/D55495</a><br>
<br>
Modified:<br>
cfe/trunk/lib/AST/ASTDumper.cpp<br>
cfe/trunk/test/AST/ast-dump-stmt.cpp<br>
<br>
Modified: cfe/trunk/lib/AST/ASTDumper.cpp<br>
URL: <a
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348794&r1=348793&r2=348794&view=diff"
rel="noreferrer" target="_blank" moz-do-not-send="true">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ASTDumper.cpp?rev=348794&r1=348793&r2=348794&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/AST/ASTDumper.cpp (original)<br>
+++ cfe/trunk/lib/AST/ASTDumper.cpp Mon Dec 10 12:53:32 2018<br>
@@ -1951,11 +1951,12 @@ void
ASTDumper::VisitInitListExpr(const<br>
OS << " field ";<br>
NodeDumper.dumpBareDeclRef(Field);<br>
}<br>
+<br>
if (auto *Filler = ILE->getArrayFiller()) {<br>
- dumpChild([=] {<br>
- OS << "array filler";<br>
- dumpStmt(Filler);<br>
- });<br>
+ OS << " array_filler";<br>
+ NodeDumper.dumpPointer(Filler);<br>
+<br>
+ dumpStmt(Filler);<br>
}<br>
}<br>
<br>
<br>
Modified: cfe/trunk/test/AST/ast-dump-stmt.cpp<br>
URL: <a
href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-stmt.cpp?rev=348794&r1=348793&r2=348794&view=diff"
rel="noreferrer" target="_blank" moz-do-not-send="true">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/AST/ast-dump-stmt.cpp?rev=348794&r1=348793&r2=348794&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/test/AST/ast-dump-stmt.cpp (original)<br>
+++ cfe/trunk/test/AST/ast-dump-stmt.cpp Mon Dec 10 12:53:32
2018<br>
@@ -90,9 +90,8 @@ void TestUnionInitList()<br>
{<br>
U us[3] = {1};<br>
// CHECK: VarDecl {{.+}} <col:3, col:15> col:5 us 'U
[3]' cinit<br>
-// CHECK-NEXT: `-InitListExpr {{.+}} <col:13, col:15>
'U [3]'<br>
-// CHECK-NEXT: |-array filler<br>
-// CHECK-NEXT: | `-InitListExpr {{.+}} <col:15> 'U'
field Field {{.+}} 'i' 'int'<br>
+// CHECK-NEXT: `-InitListExpr {{.+}} <col:13, col:15>
'U [3]' array_filler 0x{{.+}}<br>
+// CHECK-NEXT: |-InitListExpr {{.+}} <col:15> 'U'
field Field {{.+}} 'i' 'int'<br>
// CHECK-NEXT: `-InitListExpr {{.+}} <col:14> 'U'
field Field {{.+}} 'i' 'int'<br>
// CHECK-NEXT: `-IntegerLiteral {{.+}} <col:14>
'int' 1<br>
}</blockquote>
<div><br>
</div>
<div>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.</div>
<div><br>
</div>
<div>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:</div>
<div><br>
</div>
<div>// CHECK: VarDecl {{.+}} <col:3, col:15> col:5 us
'U [3]' cinit<br>
// CHECK-NEXT: `-InitListExpr {{.+}} <col:13, col:15>
'U [3]'<br>
// CHECK-NEXT: |-array filler: InitListExpr {{.+}}
<col:15> 'U' field Field {{.+}} 'i' 'int'<br>
// CHECK-NEXT: `-InitListExpr {{.+}} <col:14> 'U'
field Field {{.+}} 'i' 'int'<br>
// CHECK-NEXT: `-IntegerLiteral {{.+}} <col:14>
'int' 1<br>
</div>
<div><br>
</div>
<div>... 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.</div>
</div>
</div>
</blockquote>
</body>
</html>