[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL348795: Re-order content in OMPDeclareReductionDecl dump (authored by steveire, committed by ). Herald added a

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

ABataev added inline comments. Comment at: lib/AST/ASTDumper.cpp:1056 } +NodeDumper.dumpPointer(Initializer); + } Better to output it immediately after `initializer` keyword. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM with a small nit. @ABataev, are you okay with this approach? Comment at: lib/AST/ASTDumper.cpp:1060 + dumpStmt(D->getCombiner()); + if (auto *Initializer = D->getInitializer()) {

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

steveire updated this revision to Diff 177428. steveire added a comment. Use pointer approach Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55395/new/ https://reviews.llvm.org/D55395 Files: lib/AST/ASTDumper.cpp test/AST/dump.cpp Index: test/AST/dump.cpp

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

steveire added a comment. I would prefer *not* to do what is in this patch. I'm just trying to find a way forward. I like the approach of using the existing precedent of printing the pointers as a compromise between removing such labels entirely and not having things like that at all. I did

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

riccibruno added a comment. In D55395#1324699 , @aaron.ballman wrote: > This is a novel approach that's not used anywhere else in the AST dumper and > there are several ways we could handle this, including: > > - What's proposed (adding a new node to

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

steveire added a comment. In D55395#1324699 , @aaron.ballman wrote: > This is a novel approach that's not used anywhere else in the AST dumper Actually it's used by the InitListExpr dump. See https://reviews.llvm.org/D55488 Repository: rC Clang

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

aaron.ballman added a comment. This is a novel approach that's not used anywhere else in the AST dumper and there are several ways we could handle this, including: - What's proposed (adding a new node to the tree that's not directly an AST node) - Making use of the pointer information. e.g.,

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

steveire updated this revision to Diff 177420. steveire added a comment. Use child node labels Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55395/new/ https://reviews.llvm.org/D55395 Files: lib/AST/ASTDumper.cpp test/AST/dump.cpp Index: test/AST/dump.cpp

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

ABataev added a comment. In D55395#1322431 , @rsmith wrote: > In D55395#1322244 , @ABataev wrote: > > > This is wrong, the original implementation is correct and should not be > > changed. > > > The original

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

rsmith added a comment. In D55395#1322244 , @ABataev wrote: > This is wrong, the original implementation is correct and should not be > changed. The original implementation looks pretty clearly wrong to me, but I think this is wrong too. It looks like

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

ABataev requested changes to this revision. ABataev added a comment. This revision now requires changes to proceed. This is wrong, the original implementation is correct and should not be changed. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55395/new/

[PATCH] D55395: Re-order content in OMPDeclareReductionDecl dump

steveire created this revision. steveire added a reviewer: aaron.ballman. Herald added a subscriber: cfe-commits. Repository: rC Clang https://reviews.llvm.org/D55395 Files: lib/AST/ASTDumper.cpp test/AST/dump.cpp Index: test/AST/dump.cpp