Fix `isInfixBinaryOp` that returned true for postfix ++

[eduucaldas]

nits

[eduucaldas]

clang/unittests/Tooling/CXXOperatorCallExprTest.cpp

2

This file is in the unittests/Tooling instead in the unittests/AST directory because I wanted to have access to the TestVisitor infrastructure. I know the solution is not optimal and I am looking for suggestions

30

Previous code do something else. Look at CallsVisitor in clang/unittests/Tooling/SourceCodeTest.cpp . I prefer this approach because

• The whole logic is then inside the the test case
• There is not a big addition to lines of code

[eduucaldas]

better comment

[eduucaldas]

Nits

[eduucaldas]

Remove review formatting noise

[gribozavr2]

clang/unittests/Tooling/CXXOperatorCallExprTest.cpp

2

Could you make a separate patch where you move TestVisitor.h under clang/include/clang/Testing? Then you can place this test into unittests/AST.

OTOH, since the test is not testing the visitation, WDYT about changing the test to use AST matchers instead? It would seem to lead to more straightforward tests: get the AST node, act on it. See clang/unittests/AST/MatchVerifier.h.

21

Please add a space before "{".

24

Please add a space before "{". (Throughout the patch.)

clang/include/clang/AST/ExprCXX.h

static bool isComparisonOp(OverloadedOperatorKind Opc) {
     case OO_Spaceship:
       return true;
     default:
       return false;
     }
   }
   bool isComparisonOp() const { return isComparisonOp(getOperator()); }
 
-  /// Is this written as an infix binary operator?
   bool isInfixBinaryOp() const;
 
   /// Returns the location of the operator symbol in the expression.
   ///
   /// When \c getOperator()==OO_Call, this is the location of the right
   /// parentheses; when \c getOperator()==OO_Subscript, this is the location
   /// of the right bracket.
   SourceLocation getOperatorLoc() const { return getRParenLoc(); }

clang/lib/AST/ExprCXX.cpp

 
 using namespace clang;
 
 //===----------------------------------------------------------------------===//
 //  Child Iterators for iterating over subexpressions/substatements
 //===----------------------------------------------------------------------===//
 
 bool CXXOperatorCallExpr::isInfixBinaryOp() const {
-  // An infix binary operator is any operator with two arguments other than
-  // operator() and operator[]. Note that none of these operators can have
-  // default arguments, so it suffices to check the number of argument
-  // expressions.
   if (getNumArgs() != 2)
     return false;
 
   switch (getOperator()) {
-  case OO_Call: case OO_Subscript:
+  // operator() may have two arguments, but it's not a binary operator
+  case OO_Call:
+  // operator[] takes two arguments but it's not infix
+  case OO_Subscript:
+  // Postfix unary operators (++ and --) take 2 arguments to differ from their
+  // prefix counterparts
+  case OO_PlusPlus:
+  case OO_MinusMinus:
     return false;
   default:
     return true;
   }
 }
 
 CXXRewrittenBinaryOperator::DecomposedForm
 CXXRewrittenBinaryOperator::getDecomposedForm() const {

clang/unittests/Tooling/CMakeLists.txt

if (MSVC)
   set_source_files_properties(SourceCodeTest.cpp PROPERTIES COMPILE_FLAGS /bigobj)
 endif()
 
 add_clang_unittest(ToolingTests
   ASTSelectionTest.cpp
   CastExprTest.cpp
   CommentHandlerTest.cpp
   CompilationDatabaseTest.cpp
+  CXXOperatorCallExprTest.cpp
   DependencyScannerTest.cpp
   DiagnosticsYamlTest.cpp
   ExecutionTest.cpp
   FixItTest.cpp
   HeaderIncludesTest.cpp
   LexicallyOrderedRecursiveASTVisitorTest.cpp
   LookupTest.cpp
   QualTypeNamesTest.cpp

clang/unittests/Tooling/CXXOperatorCallExprTest.cpp

+//===- unittests/Tooling/CXXOperatorCallExprTest.cpp ----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Unit tests for the predicates in CXXOperatorCallExpr.
+//
+//===----------------------------------------------------------------------===//
+
+#include "TestVisitor.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace {
+
+TEST(CXXOperatorPredicatesTest, InfixBinaryOp) {
+  const std::string Code = R"cpp(
+  struct X{
+    friend X operator+(X, X);
+  };
+  void test(X x){
+    x + x;
+  }
+  )cpp";
+
+  struct Visitor : TestVisitor<Visitor> {
+    bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr *E) {
+      EXPECT_TRUE(E->isInfixBinaryOp());
+      return true;
+    }
+  } visitor;
+  visitor.runOver(Code);
+}
+
+TEST(CXXOperatorPredicatesTest, CallLikeOp) {
+  const std::string Code = R"cpp(
+  struct X{
+    int operator[](int idx);
+  };
+  void test(X x){
+    x[1];
+  }
+  )cpp";
+  struct Visitor : TestVisitor<Visitor> {
+    bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr *E) {
+      EXPECT_FALSE(E->isInfixBinaryOp());
+      return true;
+    }
+  } visitor;
+
+  visitor.runOver(Code);
+}
+
+TEST(CXXOperatorPredicatesTest, PostfixUnaryOp) {
+  const std::string Code = R"cpp(
+  struct X{
+    X operator++(int);
+  };
+  void test(X x){
+    x++;
+  }
+  )cpp";
+
+  struct Visitor : TestVisitor<Visitor> {
+    bool VisitCXXOperatorCallExpr(CXXOperatorCallExpr *E) {
+      EXPECT_FALSE(E->isInfixBinaryOp());
+      return true;
+    }
+  } visitor;
+
+  visitor.runOver(Code);
+}
+} // namespace
+} // namespace clang