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

[Aaron Ballman via Phabricator via cfe-commits]

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., https://pastebin.com/mh9dHT9L
- Adding the label before the AST node. e.g., https://pastebin.com/L8YwJTqe
- Adding the label after the AST node. e.g., https://pastebin.com/gbNahjsd
- Probably others

Why this way?

I'm not a huge fan of adding a new node to the tree that's not an AST node. It expands the tree both vertically (by adding a new node) and horizontally (by indenting everything below that new node) which I find visually distracting for the benefit provided. I personally prefer using the pointer information as it's less structurally disruptive and still provides the same information. I also find it a bit easier to match nodes up that way because the indentation level and tree-like adornments sometimes make it hard for me to determine relationships between when spatially far apart in the tree. There is precedence for using labels + pointers in the AST dumper already -- this is how we handle the prev and parent nodes for declarations, for instance.

If we're going to invent a novel way to do this, I do not think it should be done in an ad hoc manner, but should instead talk about whether we want to see this done in a more uniform fashion. For instance, how is this information any different than the list of overrides for a method, the previous declaration in a redecl, the parent of an out-of-line function definition, where a default template argument is inherited from, etc (all of which use pointers for relationships)? I don't feel the same way if we go with an existing practice that incrementally improves consistency.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55395/new/

https://reviews.llvm.org/D55395