[clang-diff] Fix the html output for CXXOperatorCallExpr

[johannes]

Summary

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

[arphaman]

test/Tooling/clang-diff-ast.cpp

42 ↗

(On Diff #112136)

Looks like an unrelated change.

113 ↗

(On Diff #112136)

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

[johannes]

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

[arphaman]

LGTM with one request below:

tools/clang-diff/ClangDiff.cpp

319

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

[johannes]

tools/clang-diff/ClangDiff.cpp

319

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.

[arphaman]

tools/clang-diff/ClangDiff.cpp

319

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.

[shafik]

Is this relevant anymore or should we close it?

test/Tooling/Inputs/clang-diff-basic-src.cpp

U visit(T &t) {
   return U();
 }
 
 void tmp() {
   int x;
   visit<int>(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;

test/Tooling/clang-diff-basic.cpp

-// 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
 namespace dst {
 // CHECK-NOT: Match NamespaceDecl: src{{.*}} to NamespaceDecl: inner
 namespace inner {
 void foo() {
   // CHECK: Match IntegerLiteral: 321{{.*}} to IntegerLiteral: 322

 }
 
 void tmp() {
   long x;
   // CHECK: Update TemplateArgument: int{{.*}} to long
   visit<long>(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

test/Tooling/clang-diff-html.test

-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: <pre><div id='L' class='code'><span id='L0' tid='R0' title='TranslationUnitDecl
 CHECK-NEXT: 0 -> 0'>
 
 match, update
 CHECK: <span id='L[[L:[0-9]+]]' tid='R[[R:[0-9]+]]' title='NamespaceDecl
 CHECK-NEXT: [[L]] -> [[R]]
 CHECK-NEXT: src;' class='u'>namespace src {

 update + move
 CHECK: 2' class='u m'>2</span>
 
 insertion
 CHECK: <span id='R[[R:[0-9]+]]' tid='L-1' title='StringLiteral
 CHECK-NEXT: -1 -> [[R]]
 CHECK-NEXT: Bar' class='i'>&quot;Bar&quot;</span>
 
+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

tools/clang-diff/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   Support
   )
 
 add_clang_executable(clang-diff
   ClangDiff.cpp
   )
 
 target_link_libraries(clang-diff
+  clangAST
   clangBasic
   clangFrontend
   clangTooling
   clangToolingASTDiff
   )

tools/clang-diff/ClangDiff.cpp

 //===- ClangDiff.cpp - compare source files by AST nodes ------*- C++ -*- -===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements a tool for syntax tree based comparison using
 // Tooling/ASTDiff.
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/ExprCXX.h"
 #include "clang/Tooling/ASTDiff/ASTDiff.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Program.h"
 
 using namespace llvm;
 using namespace clang;
if (!Value.empty()) {
     OS << "\n";
     printHtml(OS, Value);
   }
   OS << "'";
   if (Node.Change != diff::None)
     OS << " class='" << getChangeKindAbbr(Node.Change) << "'";
   OS << ">";
 
-  for (diff::NodeId Child : Node.Children)
+  const auto &Children = Node.Children;
+  auto *OpCall = Node.ASTNode.get<CXXOperatorCallExpr>();
+  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]);
   if (Id == Tree.getRootId()) {
     End = Code.size();
     for (; Offset < End; ++Offset)
       printHtml(OS, Code[Offset]);
   }