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. 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
