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

2018-12-10 Thread Stephen Kelly via Phabricator via cfe-commits 
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 subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D55395?vs=177428=177589#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D55395

Files:
  cfe/trunk/lib/AST/ASTDumper.cpp
  cfe/trunk/test/AST/dump.cpp


Index: cfe/trunk/test/AST/dump.cpp
===
--- cfe/trunk/test/AST/dump.cpp
+++ cfe/trunk/test/AST/dump.cpp
@@ -13,14 +13,14 @@
 
 #pragma omp declare reduction(fun : float : omp_out += omp_in) 
initializer(omp_priv = omp_orig + 15)
 
-// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 
operator+ 'int' combiner
+// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 
operator+ 'int' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'int' lvalue 
'*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_out' 'int'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
 // CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_in' 'int'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:35 implicit used omp_in 'int'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:35 implicit used omp_out 'int'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 
'char' combiner
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 
'char' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'char' 
lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_out' 'char'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
@@ -28,7 +28,7 @@
 // CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_in' 'char'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float' combiner initializer
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float' combiner 0x{{.+}} initializer 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
Index: cfe/trunk/lib/AST/ASTDumper.cpp
===
--- cfe/trunk/lib/AST/ASTDumper.cpp
+++ cfe/trunk/lib/AST/ASTDumper.cpp
@@ -1040,9 +1040,10 @@
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getType());
   OS << " combiner";
-  dumpStmt(D->getCombiner());
-  if (auto *Initializer = D->getInitializer()) {
+  NodeDumper.dumpPointer(D->getCombiner());
+  if (const auto *Initializer = D->getInitializer()) {
 OS << " initializer";
+NodeDumper.dumpPointer(Initializer);
 switch (D->getInitializerKind()) {
 case OMPDeclareReductionDecl::DirectInit:
   OS << " omp_priv = ";
@@ -1053,8 +1054,11 @@
 case OMPDeclareReductionDecl::CallInit:
   break;
 }
-dumpStmt(Initializer);
   }
+
+  dumpStmt(D->getCombiner());
+  if (const auto *Initializer = D->getInitializer())
+dumpStmt(Initializer);
 }
 
 void ASTDumper::VisitOMPRequiresDecl(const OMPRequiresDecl *D) {


Index: cfe/trunk/test/AST/dump.cpp
===
--- cfe/trunk/test/AST/dump.cpp
+++ cfe/trunk/test/AST/dump.cpp
@@ -13,14 +13,14 @@
 
 #pragma omp declare reduction(fun : float : omp_out += omp_in) initializer(omp_priv = omp_orig + 15)
 
-// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 operator+ 'int' combiner
+// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 operator+ 'int' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'int' lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 'omp_out' 'int'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
 // CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 'omp_in' 'int'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:35 implicit used omp_in 'int'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:35 implicit used omp_out 'int'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 'char' combiner
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 'char' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'char' lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 'omp_out' 'char'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
@@

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

2018-12-10 Thread Alexey Bataev via Phabricator via cfe-commits 
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
  https://reviews.llvm.org/D55395/new/

https://reviews.llvm.org/D55395



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-09 Thread Aaron Ballman via Phabricator via cfe-commits 
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()) {
 dumpStmt(Initializer);

Elide braces and may as well make this `const auto *`. (If you want to hit the 
one above, that would also be good -- maybe even hoist the call into an 
initializer for a local variable to be used in both places?)


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-09 Thread Stephen Kelly via Phabricator via cfe-commits 
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
===
--- test/AST/dump.cpp
+++ test/AST/dump.cpp
@@ -13,14 +13,14 @@
 
 #pragma omp declare reduction(fun : float : omp_out += omp_in) 
initializer(omp_priv = omp_orig + 15)
 
-// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 
operator+ 'int' combiner
+// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 
operator+ 'int' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'int' lvalue 
'*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_out' 'int'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
 // CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_in' 'int'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:35 implicit used omp_in 'int'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:35 implicit used omp_out 'int'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 
'char' combiner
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 
'char' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'char' 
lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_out' 'char'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
@@ -28,7 +28,7 @@
 // CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_in' 'char'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float' combiner initializer
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float' combiner 0x{{.+}} initializer 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1040,7 +1040,7 @@
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getType());
   OS << " combiner";
-  dumpStmt(D->getCombiner());
+  NodeDumper.dumpPointer(D->getCombiner());
   if (auto *Initializer = D->getInitializer()) {
 OS << " initializer";
 switch (D->getInitializerKind()) {
@@ -1053,6 +1053,11 @@
 case OMPDeclareReductionDecl::CallInit:
   break;
 }
+NodeDumper.dumpPointer(Initializer);
+  }
+
+  dumpStmt(D->getCombiner());
+  if (auto *Initializer = D->getInitializer()) {
 dumpStmt(Initializer);
   }
 }


Index: test/AST/dump.cpp
===
--- test/AST/dump.cpp
+++ test/AST/dump.cpp
@@ -13,14 +13,14 @@
 
 #pragma omp declare reduction(fun : float : omp_out += omp_in) initializer(omp_priv = omp_orig + 15)
 
-// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 operator+ 'int' combiner
+// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 operator+ 'int' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'int' lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 'omp_out' 'int'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
 // CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 'omp_in' 'int'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:35 implicit used omp_in 'int'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:35 implicit used omp_out 'int'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 'char' combiner
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 'char' combiner 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'char' lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 'omp_out' 'char'
 // CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
@@ -28,7 +28,7 @@
 // CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 'omp_in' 'char'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 fun 'float' combiner initializer
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 fun 'float' combiner 0x{{.+}} initializer 0x{{.+}}
 // CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}}

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

2018-12-09 Thread Stephen Kelly via Phabricator via cfe-commits 
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 that for `InitListExpr` here: 
https://reviews.llvm.org/D55495


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-09 Thread Bruno Ricci via Phabricator via cfe-commits 
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 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.


I personally also prefer the first one. However is this really needed in this 
case ? The first sub-node will always be the combiner,
and then the second node will the the initializer if present. It seems to me 
that there is no ambiguity here.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-09 Thread Stephen Kelly via Phabricator via cfe-commits 
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

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

https://reviews.llvm.org/D55395



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-09 Thread 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-09 Thread Stephen Kelly via Phabricator via cfe-commits 
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
===
--- test/AST/dump.cpp
+++ test/AST/dump.cpp
@@ -13,33 +13,37 @@
 
 #pragma omp declare reduction(fun : float : omp_out += omp_in) 
initializer(omp_priv = omp_orig + 15)
 
-// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 
operator+ 'int' combiner
-// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'int' lvalue 
'*=' ComputeLHSTy='int' ComputeResultTy='int'
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_out' 'int'
-// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
-// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_in' 'int'
+// CHECK:  |-OMPDeclareReductionDecl {{.+}}  col:35 
operator+ 'int'
+// CHECK-NEXT: | |-combiner
+// CHECK-NEXT: | | `-CompoundAssignOperator {{.+}}  'int' 
lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
+// CHECK-NEXT: | |   |-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_out' 'int'
+// CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}}  'int' 
+// CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'int' lvalue Var {{.+}} 
'omp_in' 'int'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:35 implicit used omp_in 'int'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:35 implicit used omp_out 'int'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 
'char' combiner
-// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'char' 
lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_out' 'char'
-// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'int' 
-// CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}}  'char' 
-// CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_in' 'char'
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:40 operator+ 
'char'
+// CHECK-NEXT: | |-combiner
+// CHECK-NEXT: | `-CompoundAssignOperator {{.+}}  'char' 
lvalue '*=' ComputeLHSTy='int' ComputeResultTy='int'
+// CHECK-NEXT: |   |-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_out' 'char'
+// CHECK-NEXT: |   `-ImplicitCastExpr {{.+}}  'int' 
+// CHECK-NEXT: | `-ImplicitCastExpr {{.+}}  'char' 
+// CHECK-NEXT: |   `-DeclRefExpr {{.+}}  'char' lvalue Var {{.+}} 
'omp_in' 'char'
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
-// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float' combiner initializer
-// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
-// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
-// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_in' 'float'
-// CHECK-NEXT: | |-BinaryOperator {{.+}}  'float' lvalue '='
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_priv' 'float'
-// CHECK-NEXT: | | `-BinaryOperator {{.+}}  'float' '+'
-// CHECK-NEXT: | |   |-ImplicitCastExpr {{.+}}  'float' 

-// CHECK-NEXT: | |   | `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_orig' 'float'
-// CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}}  'float' 

-// CHECK-NEXT: | | `-IntegerLiteral {{.+}}  'int' 15
+// CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float'
+// CHECK-NEXT: | |-combiner
+// CHECK-NEXT: | `-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
+// CHECK-NEXT: |   |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
+// CHECK-NEXT: |   `-ImplicitCastExpr {{.+}}  'float' 
+// CHECK-NEXT: | `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_in' 'float'
+// CHECK-NEXT: | |-initializer
+// CHECK-NEXT: | `-BinaryOperator {{.+}}  'float' lvalue '='
+// CHECK-NEXT: |   |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_priv' 'float'
+// CHECK-NEXT: |   `-BinaryOperator {{.+}}  'float' '+'
+// CHECK-NEXT: | |-ImplicitCastExpr {{.+}}  'float' 

+// CHECK-NEXT: | | `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_orig' 'float'
+// CHECK-NEXT: | `-ImplicitCastExpr {{.+}}  'float' 

+// CHECK-NEXT: |   `-IntegerLiteral {{.+}}  'int' 15
 
 struct S {
   int a, b;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1043,21 +1043,21 @@
 void ASTDumper::VisitOMPDeclareReductionDecl(const OMPDeclareReductionDecl *D) 
{
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getType());
-  OS << " combiner";
-  dumpStmt(D->getCombiner());
+
+  dumpStmt(D->getCombiner(), "combiner");
   if (auto *Initializer

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

2018-12-07 Thread Alexey Bataev via Phabricator via cfe-commits 
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 implementation looks pretty clearly wrong to me, but I think 
> this is wrong too. It looks like what was intended here was probably to label 
> the two children as "combiner" and "initializer" respectively. As-is, the 
> "combiner"  text on the `OMPDeclareReductionDecl` line means nothing at all.


Oh yes, agree. Still must be reworked.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-06 Thread Richard Smith - zygoloid via Phabricator via cfe-commits 
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 what was intended here was probably to label the 
two children as "combiner" and "initializer" respectively. As-is, the 
"combiner"  text on the `OMPDeclareReductionDecl` line means nothing at all.


Repository:
  rC Clang

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

https://reviews.llvm.org/D55395



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-06 Thread Alexey Bataev via Phabricator via cfe-commits 
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/

https://reviews.llvm.org/D55395



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-12-06 Thread Stephen Kelly via Phabricator via cfe-commits 
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
===
--- test/AST/dump.cpp
+++ test/AST/dump.cpp
@@ -29,10 +29,6 @@
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
 // CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 
fun 'float' combiner initializer
-// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
-// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
-// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_in' 'float'
 // CHECK-NEXT: | |-BinaryOperator {{.+}}  'float' lvalue '='
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_priv' 'float'
 // CHECK-NEXT: | | `-BinaryOperator {{.+}}  'float' '+'
@@ -40,6 +36,10 @@
 // CHECK-NEXT: | |   | `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_orig' 'float'
 // CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}}  'float' 

 // CHECK-NEXT: | | `-IntegerLiteral {{.+}}  'int' 15
+// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' 
lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
+// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_out' 'float'
+// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
+// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 
'omp_in' 'float'
 
 struct S {
   int a, b;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1074,7 +1074,6 @@
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getType());
   OS << " combiner";
-  dumpStmt(D->getCombiner());
   if (auto *Initializer = D->getInitializer()) {
 OS << " initializer";
 switch (D->getInitializerKind()) {
@@ -1089,6 +1088,7 @@
 }
 dumpStmt(Initializer);
   }
+  dumpStmt(D->getCombiner());
 }
 
 void ASTDumper::VisitOMPRequiresDecl(const OMPRequiresDecl *D) {


Index: test/AST/dump.cpp
===
--- test/AST/dump.cpp
+++ test/AST/dump.cpp
@@ -29,10 +29,6 @@
 // CHECK-NEXT: | |-VarDecl {{.+}}  col:40 implicit used omp_in 'char'
 // CHECK-NEXT: | `-VarDecl {{.+}}  col:40 implicit used omp_out 'char'
 // CHECK-NEXT: |-OMPDeclareReductionDecl {{.+}}  col:37 fun 'float' combiner initializer
-// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
-// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_out' 'float'
-// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
-// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_in' 'float'
 // CHECK-NEXT: | |-BinaryOperator {{.+}}  'float' lvalue '='
 // CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_priv' 'float'
 // CHECK-NEXT: | | `-BinaryOperator {{.+}}  'float' '+'
@@ -40,6 +36,10 @@
 // CHECK-NEXT: | |   | `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_orig' 'float'
 // CHECK-NEXT: | |   `-ImplicitCastExpr {{.+}}  'float' 
 // CHECK-NEXT: | | `-IntegerLiteral {{.+}}  'int' 15
+// CHECK-NEXT: | |-CompoundAssignOperator {{.+}}  'float' lvalue '+=' ComputeLHSTy='float' ComputeResultTy='float'
+// CHECK-NEXT: | | |-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_out' 'float'
+// CHECK-NEXT: | | `-ImplicitCastExpr {{.+}}  'float' 
+// CHECK-NEXT: | |   `-DeclRefExpr {{.+}}  'float' lvalue Var {{.+}} 'omp_in' 'float'
 
 struct S {
   int a, b;
Index: lib/AST/ASTDumper.cpp
===
--- lib/AST/ASTDumper.cpp
+++ lib/AST/ASTDumper.cpp
@@ -1074,7 +1074,6 @@
   NodeDumper.dumpName(D);
   NodeDumper.dumpType(D->getType());
   OS << " combiner";
-  dumpStmt(D->getCombiner());
   if (auto *Initializer = D->getInitializer()) {
 OS << " initializer";
 switch (D->getInitializerKind()) {
@@ -1089,6 +1088,7 @@
 }
 dumpStmt(Initializer);
   }
+  dumpStmt(D->getCombiner());
 }
 
 void ASTDumper::VisitOMPRequiresDecl(const OMPRequiresDecl *D) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits