[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

Looks like an unrelated change.

113

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/clang-diff-ast.cpp

 typedef unsigned nat;
 // CHECK: TypeAliasDecl: real;double;(
 using real = double;
 
 class Base {
 };
 
 // CHECK: CXXRecordDecl: X;X;(
-class X : Base {
+struct X : Base {
   int m;
   // CHECK: CXXMethodDecl: :foo(const char *(int)
   // CHECK: ParmVarDecl: i(int)(
   const char *foo(int i) {
     if (i == 0)
       // CHECK: StringLiteral: foo(
       return "foo";
     // CHECK-NOT: ImplicitCastExpr
template <class T> class C {
   // CHECK-NEXT: FieldDecl
   T t;
 };
 
 // CHECK-NEXT: CXXRecordDecl
 // CHECK-NEXT: TemplateName
 // CHECK-NEXT: TemplateArgument
 class I : C<int> {};
+
+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;

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]);
   }