[clang-tidy] Misc redundant expressions check updated for overloaded operators

Patch by: Lilla Barancsuk

Differential Revision: https://reviews.llvm.org/D39243

llvm-svn: 319033

clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp

@@ -98,6 +98,9 @@ static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
   case Stmt::StringLiteralClass:
     return cast<StringLiteral>(Left)->getBytes() ==
            cast<StringLiteral>(Right)->getBytes();
+  case Stmt::CXXOperatorCallExprClass:
+    return cast<CXXOperatorCallExpr>(Left)->getOperator() ==
+           cast<CXXOperatorCallExpr>(Right)->getOperator();
   case Stmt::DependentScopeDeclRefExprClass:
     if (cast<DependentScopeDeclRefExpr>(Left)->getDeclName() !=
         cast<DependentScopeDeclRefExpr>(Right)->getDeclName())
@@ -410,6 +413,7 @@ matchRelationalIntegerConstantExpr(StringRef Id) {
   std::string CastId = (Id + "-cast").str();
   std::string SwapId = (Id + "-swap").str();
   std::string NegateId = (Id + "-negate").str();
+  std::string OverloadId = (Id + "-overload").str();
 
   const auto RelationalExpr = ignoringParenImpCasts(binaryOperator(
       isComparisonOperator(), expr().bind(Id),
@@ -437,12 +441,54 @@ matchRelationalIntegerConstantExpr(StringRef Id) {
                         hasOperatorName("!"),
                         hasUnaryOperand(anyOf(CastExpr, RelationalExpr)))));
 
+  const auto OverloadedOperatorExpr =
+      cxxOperatorCallExpr(
+          anyOf(hasOverloadedOperatorName("=="),
+                hasOverloadedOperatorName("!="), hasOverloadedOperatorName("<"),
+                hasOverloadedOperatorName("<="), hasOverloadedOperatorName(">"),
+                hasOverloadedOperatorName(">=")),
+          // Filter noisy false positives.
+          unless(isMacro()), unless(isInTemplateInstantiation()))
+          .bind(OverloadId);
+
   return anyOf(RelationalExpr, CastExpr, NegateRelationalExpr,
-               NegateNegateRelationalExpr);
+               NegateNegateRelationalExpr, OverloadedOperatorExpr);
 }
 
-// Retrieves sub-expressions matched by 'matchRelationalIntegerConstantExpr' with
+// Checks whether a function param is non constant reference type, and may
-// name 'Id'.
+// be modified in the function.
+static bool isNonConstReferenceType(QualType ParamType) {
+  return ParamType->isReferenceType() &&
+         !ParamType.getNonReferenceType().isConstQualified();
+}
+
+// Checks whether the arguments of an overloaded operator can be modified in the
+// function.
+// For operators that take an instance and a constant as arguments, only the
+// first argument (the instance) needs to be checked, since the constant itself
+// is a temporary expression. Whether the second parameter is checked is
+// controlled by the parameter `ParamsToCheckCount`.
+static bool
+canOverloadedOperatorArgsBeModified(const FunctionDecl *OperatorDecl,
+                                    bool checkSecondParam) {
+  unsigned ParamCount = OperatorDecl->getNumParams();
+
+  // Overloaded operators declared inside a class have only one param.
+  // These functions must be declared const in order to not be able to modify
+  // the instance of the class they are called through.
+  if (ParamCount == 1 &&
+      !OperatorDecl->getType()->getAs<FunctionType>()->isConst())
+    return true;
+
+  if (isNonConstReferenceType(OperatorDecl->getParamDecl(0)->getType()))
+    return true;
+
+  return checkSecondParam && ParamCount == 2 &&
+         isNonConstReferenceType(OperatorDecl->getParamDecl(1)->getType());
+}
+
+// Retrieves sub-expressions matched by 'matchRelationalIntegerConstantExpr'
+// with name 'Id'.
 static bool retrieveRelationalIntegerConstantExpr(
     const MatchFinder::MatchResult &Result, StringRef Id,
     const Expr *&OperandExpr, BinaryOperatorKind &Opcode, const Expr *&Symbol,
@@ -450,6 +496,7 @@ static bool retrieveRelationalIntegerConstantExpr(
   std::string CastId = (Id + "-cast").str();
   std::string SwapId = (Id + "-swap").str();
   std::string NegateId = (Id + "-negate").str();
+  std::string OverloadId = (Id + "-overload").str();
 
   if (const auto *Bin = Result.Nodes.getNodeAs<BinaryOperator>(Id)) {
     // Operand received with explicit comparator.
@@ -458,12 +505,29 @@ static bool retrieveRelationalIntegerConstantExpr(
 
     if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr))
       return false;
-
   } else if (const auto *Cast = Result.Nodes.getNodeAs<CastExpr>(CastId)) {
     // Operand received with implicit comparator (cast).
     Opcode = BO_NE;
     OperandExpr = Cast;
     Value = APSInt(32, false);
+  } else if (const auto *OverloadedOperatorExpr =
+                 Result.Nodes.getNodeAs<CXXOperatorCallExpr>(OverloadId)) {
+    const auto *OverloadedFunctionDecl = dyn_cast_or_null<FunctionDecl>(OverloadedOperatorExpr->getCalleeDecl());
+    if (!OverloadedFunctionDecl)
+      return false;
+
+    if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, false))
+      return false;
+
+    if (!OverloadedOperatorExpr->getArg(1)->isIntegerConstantExpr(
+            Value, *Result.Context))
+      return false;
+
+    Symbol = OverloadedOperatorExpr->getArg(0);
+    OperandExpr = OverloadedOperatorExpr;
+    Opcode = BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator());
+
+    return BinaryOperator::isComparisonOp(Opcode);
   } else {
     return false;
   }
@@ -548,7 +612,8 @@ static bool areExprsFromDifferentMacros(const Expr *LhsExpr,
           Lexer::getImmediateMacroName(RhsLoc, SM, LO));
 }
 
-static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr, const Expr *&RhsExpr) {
+static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
+                                     const Expr *&RhsExpr) {
   if (!LhsExpr || !RhsExpr)
     return false;
 
@@ -562,7 +627,8 @@ void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
   const auto AnyLiteralExpr = ignoringParenImpCasts(
       anyOf(cxxBoolLiteral(), characterLiteral(), integerLiteral()));
 
-  const auto BannedIntegerLiteral = integerLiteral(expandedByMacro(KnownBannedMacroNames));
+  const auto BannedIntegerLiteral =
+      integerLiteral(expandedByMacro(KnownBannedMacroNames));
 
   // Binary with equivalent operands, like (X != 2 && X != 2).
   Finder->addMatcher(
@@ -584,13 +650,12 @@ void RedundantExpressionCheck::registerMatchers(MatchFinder *Finder) {
       this);
 
   // Conditional (trenary) operator with equivalent operands, like (Y ? X : X).
-  Finder->addMatcher(
+  Finder->addMatcher(conditionalOperator(expressionsAreEquivalent(),
-      conditionalOperator(expressionsAreEquivalent(),
+                                         // Filter noisy false positives.
-                          // Filter noisy false positives.
+                                         unless(conditionalOperatorIsInMacro()),
-                          unless(conditionalOperatorIsInMacro()),
+                                         unless(isInTemplateInstantiation()))
-                          unless(isInTemplateInstantiation()))
+                         .bind("cond"),
-          .bind("cond"),
+                     this);
-      this);
 
   // Overloaded operators with equivalent operands.
   Finder->addMatcher(
@@ -821,16 +886,15 @@ void RedundantExpressionCheck::checkRelationalExpr(
 
 void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
   if (const auto *BinOp = Result.Nodes.getNodeAs<BinaryOperator>("binary")) {
-
     // If the expression's constants are macros, check whether they are
     // intentional.
     if (areSidesBinaryConstExpressions(BinOp, Result.Context)) {
       const Expr *LhsConst = nullptr, *RhsConst = nullptr;
       BinaryOperatorKind MainOpcode, SideOpcode;
 
-      if(!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode, LhsConst,
+      if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode,
-                                     RhsConst, Result.Context))
+                                          LhsConst, RhsConst, Result.Context))
-          return;
+        return;
 
       if (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
           areExprsMacroAndNonMacro(LhsConst, RhsConst))
@@ -853,6 +917,13 @@ void RedundantExpressionCheck::check(const MatchFinder::MatchResult &Result) {
   }
 
   if (const auto *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("call")) {
+    const auto *OverloadedFunctionDecl = dyn_cast_or_null<FunctionDecl>(Call->getCalleeDecl());
+    if (!OverloadedFunctionDecl)
+      return;
+
+    if (canOverloadedOperatorArgsBeModified(OverloadedFunctionDecl, true))
+      return;
+
     diag(Call->getOperatorLoc(),
          "both sides of overloaded operator are equivalent");
   }

clang-tools-extra/test/clang-tidy/misc-redundant-expression.cpp

@@ -124,16 +124,36 @@ int TestConditional(int x, int y) {
 #undef COND_OP_MACRO
 #undef COND_OP_OTHER_MACRO
 
+// Overloaded operators that compare two instances of a struct.
 struct MyStruct {
-  int x;
+  int x;  
+  bool operator==(const MyStruct& rhs) const {return this->x == rhs.x; } // not modifing
+  bool operator>=(const MyStruct& rhs) const { return this->x >= rhs.x; } // not modifing
+  bool operator<=(MyStruct& rhs) const { return this->x <= rhs.x; }
+  bool operator&&(const MyStruct& rhs){ this->x++; return this->x && rhs.x; }
 } Q;
 
-bool operator==(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; }
+bool operator!=(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x == rhs.x; } // not modifing
+bool operator<(const MyStruct& lhs, const MyStruct& rhs) { return lhs.x < rhs.x; } // not modifing
+bool operator>(const MyStruct& lhs, MyStruct& rhs) { rhs.x--; return lhs.x > rhs.x; }
+bool operator||(MyStruct& lhs, const MyStruct& rhs) { lhs.x++; return lhs.x || rhs.x; }
 
-bool TestOperator(MyStruct& S) {
+bool TestOverloadedOperator(MyStruct& S) {
   if (S == Q) return false;
+
+  if (S <= S) return false;
+  if (S && S) return false;
+  if (S > S) return false;
+  if (S || S) return false;
+
   if (S == S) return true;
   // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
+  if (S < S) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
+  if (S != S) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
+  if (S >= S) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:9: warning: both sides of overloaded operator are equivalent
 }
 
 #define LT(x, y) (void)((x) < (y))
@@ -176,7 +196,7 @@ template <typename T, typename U>
 void TemplateCheck() {
   static_assert(T::Value == U::Value, "should be identical");
   static_assert(T::Value == T::Value, "should be identical");
-  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of overloaded operator are equivalent
+  // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: both sides of operator are equivalent
 }
 void TestTemplate() { TemplateCheck<MyClass, MyClass>(); }
 
@@ -281,9 +301,46 @@ int TestBitwise(int X, int Y) {
   return 0;
 }
 
+// Overloaded operators that compare an instance of a struct and an integer
+// constant.
+struct S {
+  S() { x = 1; }
+  int x;
+  // Overloaded comparison operators without any possible side effect.
+  bool operator==(const int &i) const { return x == i; } // not modifying
+  bool operator!=(int i) const { return x != i; } // not modifying
+  bool operator>(const int &i) const { return x > i; } // not modifying
+  bool operator<(int i) const { return x < i; } // not modifying
+};
+
+bool operator<=(const S &s, int i) { return s.x <= i; } // not modifying
+bool operator>=(const S &s, const int &i) { return s.x >= i; } // not modifying
+
+struct S2 {
+  S2() { x = 1; }
+  int x;
+  // Overloaded comparison operators that are able to modify their params.
+  bool operator==(const int &i) {
+    this->x++;
+    return x == i;
+  }
+  bool operator!=(int i) { return x != i; }
+  bool operator>(const int &i) { return x > i; }
+  bool operator<(int i) {
+    this->x--;
+    return x < i;
+  }
+};
+
+bool operator>=(S2 &s, const int &i) { return s.x >= i; }
+bool operator<=(S2 &s, int i) {
+  s.x++;
+  return s.x <= i;
+}
+
 int TestLogical(int X, int Y){
 #define CONFIG 0
-  if (CONFIG && X) return 1; // OK, consts from macros are considered intentional
+  if (CONFIG && X) return 1;
 #undef CONFIG
 #define CONFIG 1
   if (CONFIG || X) return 1;
@@ -331,6 +388,24 @@ int TestLogical(int X, int Y){
   if (!X && Y) return 1;
   if (!X && Y == 0) return 1;
   if (X == 10 && Y != 10) return 1;
+
+  // Test for overloaded operators with constant params.
+  S s1;
+  if (s1 == 1 && s1 == 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: equivalent expression on both sides of logical operator
+  if (s1 == 1 || s1 != 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+  if (s1 > 1 && s1 < 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+  if (s1 >= 1 || s1 <= 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+
+  // Test for overloaded operators that may modify their params.
+  S2 s2;
+  if (s2 == 1 || s2 != 1) return true;
+  if (s2 == 1 || s2 == 1) return true;
+  if (s2 > 1 && s2 < 1) return true;
+  if (s2 >= 1 || s2 <= 1) return true;
 }
 
 int TestRelational(int X, int Y) {