[PATCH] D37004: [clang-diff] Fix the html output for CXXOperatorCallExpr

2023-09-21 Thread Shafik Yaghmour via Phabricator via cfe-commits 
shafik added a comment.
Herald added a project: All.

Is this relevant anymore or should we close it?


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

https://reviews.llvm.org/D37004

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

[PATCH] D37004: [clang-diff] Fix the html output for CXXOperatorCallExpr

2017-08-25 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman added inline comments.



Comment at: tools/clang-diff/ClangDiff.cpp:319
+   "A Binary operator is supposed to have two arguments.");
+for (int I : {1, 0, 2})
+  Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Children[I], Offset);

johannes wrote:
> arphaman wrote:
> > Please add a short comment that describes why this out-of-order traversal 
> > is required
> Should we do this in LexicallyOrderedRecursiveASTVisitor?
> 
> There are some other cases with CXXOperatorCallExpr where the order needs to 
> be changed, e.g. postfix operators, operator->, operator() and operator[].
> It can be done by sorting by SourceLocation of the first two elements, as the 
> operator is always the first one.
Sure, that would make sense. You probably don't need to sort, just figure out 
the right order from the specific combination of operator and infix/postfix.


https://reviews.llvm.org/D37004



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

[PATCH] D37004: [clang-diff] Fix the html output for CXXOperatorCallExpr

2017-08-25 Thread Johannes Altmanninger via Phabricator via cfe-commits 
johannes added inline comments.



Comment at: tools/clang-diff/ClangDiff.cpp:319
+   "A Binary operator is supposed to have two arguments.");
+for (int I : {1, 0, 2})
+  Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Children[I], Offset);

arphaman wrote:
> Please add a short comment that describes why this out-of-order traversal is 
> required
Should we do this in LexicallyOrderedRecursiveASTVisitor?

There are some other cases with CXXOperatorCallExpr where the order needs to be 
changed, e.g. postfix operators, operator->, operator() and operator[].
It can be done by sorting by SourceLocation of the first two elements, as the 
operator is always the first one.


https://reviews.llvm.org/D37004



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

[PATCH] D37004: [clang-diff] Fix the html output for CXXOperatorCallExpr

2017-08-24 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman accepted this revision.
arphaman added a comment.
This revision is now accepted and ready to land.

LGTM with one request below:




Comment at: tools/clang-diff/ClangDiff.cpp:319
+   "A Binary operator is supposed to have two arguments.");
+for (int I : {1, 0, 2})
+  Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Children[I], Offset);

Please add a short comment that describes why this out-of-order traversal is 
required


https://reviews.llvm.org/D37004



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

[PATCH] D37004: [clang-diff] Fix the html output for CXXOperatorCallExpr

2017-08-23 Thread Johannes Altmanninger via Phabricator via cfe-commits 
johannes updated this revision to Diff 112416.
johannes added a comment.

provide a test
need to update the maxsize parameter to -s=200 here because the test file is
expected to fit within this size


https://reviews.llvm.org/D37004

Files:
  test/Tooling/Inputs/clang-diff-basic-src.cpp
  test/Tooling/clang-diff-basic.cpp
  test/Tooling/clang-diff-html.test
  tools/clang-diff/CMakeLists.txt
  tools/clang-diff/ClangDiff.cpp

Index: tools/clang-diff/ClangDiff.cpp
===
--- tools/clang-diff/ClangDiff.cpp
+++ tools/clang-diff/ClangDiff.cpp
@@ -12,6 +12,7 @@
 //
 //===--===//
 
+#include "clang/AST/ExprCXX.h"
 #include "clang/Tooling/ASTDiff/ASTDiff.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
@@ -310,8 +311,17 @@
 OS << " class='" << getChangeKindAbbr(Node.Change) << "'";
   OS << ">";
 
-  for (diff::NodeId Child : Node.Children)
-Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Child, Offset);
+  const auto  = Node.Children;
+  auto *OpCall = Node.ASTNode.get();
+  if (OpCall && OpCall->isInfixBinaryOp()) {
+assert(Children.size() == 3 &&
+   "A Binary operator is supposed to have two arguments.");
+for (int I : {1, 0, 2})
+  Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Children[I], Offset);
+  } else {
+for (diff::NodeId Child : Children)
+  Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Child, Offset);
+  }
 
   for (; Offset < End; ++Offset)
 printHtml(OS, Code[Offset]);
Index: tools/clang-diff/CMakeLists.txt
===
--- tools/clang-diff/CMakeLists.txt
+++ tools/clang-diff/CMakeLists.txt
@@ -7,6 +7,7 @@
   )
 
 target_link_libraries(clang-diff
+  clangAST
   clangBasic
   clangFrontend
   clangTooling
Index: test/Tooling/clang-diff-html.test
===
--- test/Tooling/clang-diff-html.test
+++ test/Tooling/clang-diff-html.test
@@ -1,4 +1,4 @@
-RUN: clang-diff -html %S/Inputs/clang-diff-basic-src.cpp %S/clang-diff-basic.cpp -- -std=c++11 | FileCheck %s
+RUN: clang-diff -html %S/Inputs/clang-diff-basic-src.cpp %S/clang-diff-basic.cpp -s=200 -- -std=c++11 | FileCheck %s
 
 CHECK: 
@@ -31,6 +31,13 @@
 CHECK-NEXT: -1 -> [[R]]
 CHECK-NEXT: Bar' class='i'>Bar
 
+CHECK: CXXOperatorCallExpr
+CHECK: CXXOperatorCallExpr
+CHECK: s1
+CHECK: str::operator+
+CHECK: s2
+CHECK: str::operator-
+CHECK: s3
+
 comments
-CHECK: // CHECK: Insert IfStmt{{.*}} into IfStmt
 CHECK: // CHECK: Delete AccessSpecDecl: public
Index: test/Tooling/clang-diff-basic.cpp
===
--- test/Tooling/clang-diff-basic.cpp
+++ test/Tooling/clang-diff-basic.cpp
@@ -1,4 +1,4 @@
-// RUN: clang-diff -dump-matches %S/Inputs/clang-diff-basic-src.cpp %s -- -std=c++11 | FileCheck %s
+// RUN: clang-diff -dump-matches %S/Inputs/clang-diff-basic-src.cpp %s -s=200 -- -std=c++11 | FileCheck %s
 
 // CHECK: Match TranslationUnitDecl{{.*}} to TranslationUnitDecl
 // CHECK: Match NamespaceDecl: src{{.*}} to NamespaceDecl: dst
@@ -81,5 +81,21 @@
   visit(x);
 }
 
+struct str {
+  str& operator+(const str&);
+  str& operator-(const str&);
+  str& operator++();
+  str operator++(int);
+} s1, s2, s3;
+
+// CHECK: Match CXXOperatorCallExpr
+// CHECK-NEXT: Match DeclRefExpr: str::operator-{{.*}} to DeclRefExpr: str::operator-
+// CHECK-NEXT: Match CXXOperatorCallExpr
+// CHECK-NEXT: Match DeclRefExpr: str::operator+{{.*}} to DeclRefExpr: str::operator+
+// CHECK-NEXT: Match DeclRefExpr: s1
+// CHECK-NEXT: Match DeclRefExpr: s2
+// CHECK-NEXT: Match DeclRefExpr: s3
+str x = s1 + s2 - s3;
+
 // CHECK: Delete AccessSpecDecl: public
 // CHECK: Delete CXXMethodDecl
Index: test/Tooling/Inputs/clang-diff-basic-src.cpp
===
--- test/Tooling/Inputs/clang-diff-basic-src.cpp
+++ test/Tooling/Inputs/clang-diff-basic-src.cpp
@@ -53,4 +53,11 @@
   visit(x);
 }
 
-int x = 1;
+struct str {
+  str& operator+(const str&);
+  str& operator-(const str&);
+  str& operator++();
+  str operator++(int);
+} s1, s2, s3;
+
+str x = s1 + s2 - s3;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37004: [clang-diff] Fix the html output for CXXOperatorCallExpr

2017-08-23 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman added inline comments.



Comment at: test/Tooling/clang-diff-ast.cpp:42
 // CHECK: CXXRecordDecl: X;X;(
-class X : Base {
+struct X : Base {
   int m;

Looks like an unrelated change.


https://reviews.llvm.org/D37004



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

[PATCH] D37004: [clang-diff] Fix the html output for CXXOperatorCallExpr

2017-08-23 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman added inline comments.



Comment at: test/Tooling/clang-diff-ast.cpp:113
+
+// CHECK: CXXOperatorCallExpr
+// CHECK-NEXT: DeclRefExpr: str::operator+

This test doesn't work because it passes prior to this patch. You should test 
the HTML output instead.


https://reviews.llvm.org/D37004



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

[PATCH] D37004: [clang-diff] Fix the html output for CXXOperatorCallExpr

2017-08-22 Thread Johannes Altmanninger via Phabricator via cfe-commits 
johannes created this revision.
Herald added subscribers: mgorny, klimek.

The operator is always the first child of such an expression.
If it is an infix operator, we want to print the LHS first.


https://reviews.llvm.org/D37004

Files:
  test/Tooling/clang-diff-ast.cpp
  tools/clang-diff/CMakeLists.txt
  tools/clang-diff/ClangDiff.cpp


Index: tools/clang-diff/ClangDiff.cpp
===
--- tools/clang-diff/ClangDiff.cpp
+++ tools/clang-diff/ClangDiff.cpp
@@ -12,6 +12,7 @@
 //
 
//===--===//
 
+#include "clang/AST/ExprCXX.h"
 #include "clang/Tooling/ASTDiff/ASTDiff.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
@@ -310,8 +311,17 @@
 OS << " class='" << getChangeKindAbbr(Node.Change) << "'";
   OS << ">";
 
-  for (diff::NodeId Child : Node.Children)
-Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Child, Offset);
+  const auto  = Node.Children;
+  auto *OpCall = Node.ASTNode.get();
+  if (OpCall && OpCall->isInfixBinaryOp()) {
+assert(Children.size() == 3 &&
+   "A Binary operator is supposed to have two arguments.");
+for (int I : {1, 0, 2})
+  Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Children[I], Offset);
+  } else {
+for (diff::NodeId Child : Children)
+  Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Child, Offset);
+  }
 
   for (; Offset < End; ++Offset)
 printHtml(OS, Code[Offset]);
Index: tools/clang-diff/CMakeLists.txt
===
--- tools/clang-diff/CMakeLists.txt
+++ tools/clang-diff/CMakeLists.txt
@@ -7,6 +7,7 @@
   )
 
 target_link_libraries(clang-diff
+  clangAST
   clangBasic
   clangFrontend
   clangTooling
Index: test/Tooling/clang-diff-ast.cpp
===
--- test/Tooling/clang-diff-ast.cpp
+++ test/Tooling/clang-diff-ast.cpp
@@ -39,7 +39,7 @@
 };
 
 // CHECK: CXXRecordDecl: X;X;(
-class X : Base {
+struct X : Base {
   int m;
   // CHECK: CXXMethodDecl: :foo(const char *(int)
   // CHECK: ParmVarDecl: i(int)(
@@ -105,3 +105,23 @@
 // CHECK-NEXT: TemplateName
 // CHECK-NEXT: TemplateArgument
 class I : C {};
+
+struct str {
+  str& operator+(const str&);
+} s1, s2, s3;
+
+// CHECK: CXXOperatorCallExpr
+// CHECK-NEXT: DeclRefExpr: str::operator+
+// CHECK-NEXT: CXXOperatorCallExpr
+// CHECK-NEXT: DeclRefExpr: str::operator+
+// CHECK-NEXT: DeclRefExpr: s1
+// CHECK-NEXT: DeclRefExpr: s2
+// CHECK-NEXT: DeclRefExpr: s3
+str x = s1 + s2 + s3;
+
+// CHECK: BinaryOperator: +
+// CHECK-NEXT: BinaryOperator: +
+// CHECK-NEXT: IntegerLiteral: 1
+// CHECK-NEXT: IntegerLiteral: 2
+// CHECK-NEXT: IntegerLiteral: 3
+int op = 1 + 2 + 3;


Index: tools/clang-diff/ClangDiff.cpp
===
--- tools/clang-diff/ClangDiff.cpp
+++ tools/clang-diff/ClangDiff.cpp
@@ -12,6 +12,7 @@
 //
 //===--===//
 
+#include "clang/AST/ExprCXX.h"
 #include "clang/Tooling/ASTDiff/ASTDiff.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
@@ -310,8 +311,17 @@
 OS << " class='" << getChangeKindAbbr(Node.Change) << "'";
   OS << ">";
 
-  for (diff::NodeId Child : Node.Children)
-Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Child, Offset);
+  const auto  = Node.Children;
+  auto *OpCall = Node.ASTNode.get();
+  if (OpCall && OpCall->isInfixBinaryOp()) {
+assert(Children.size() == 3 &&
+   "A Binary operator is supposed to have two arguments.");
+for (int I : {1, 0, 2})
+  Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Children[I], Offset);
+  } else {
+for (diff::NodeId Child : Children)
+  Offset = printHtmlForNode(OS, Diff, Tree, IsLeft, Child, Offset);
+  }
 
   for (; Offset < End; ++Offset)
 printHtml(OS, Code[Offset]);
Index: tools/clang-diff/CMakeLists.txt
===
--- tools/clang-diff/CMakeLists.txt
+++ tools/clang-diff/CMakeLists.txt
@@ -7,6 +7,7 @@
   )
 
 target_link_libraries(clang-diff
+  clangAST
   clangBasic
   clangFrontend
   clangTooling
Index: test/Tooling/clang-diff-ast.cpp
===
--- test/Tooling/clang-diff-ast.cpp
+++ test/Tooling/clang-diff-ast.cpp
@@ -39,7 +39,7 @@
 };
 
 // CHECK: CXXRecordDecl: X;X;(
-class X : Base {
+struct X : Base {
   int m;
   // CHECK: CXXMethodDecl: :foo(const char *(int)
   // CHECK: ParmVarDecl: i(int)(
@@ -105,3 +105,23 @@
 // CHECK-NEXT: TemplateName
 // CHECK-NEXT: TemplateArgument
 class I : C {};
+
+struct str {
+  str& operator+(const str&);
+} s1, s2, s3;
+
+// CHECK: CXXOperatorCallExpr
+// CHECK-NEXT: DeclRefExpr: str::operator+
+// CHECK-NEXT: CXXOperatorCallExpr
+// CHECK-NEXT: DeclRefExpr: str::operator+
+//