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

[barancsuk]

Summary

Redundant Expression Checker is updated to be able to match redundant expressions that contain binary overloaded operators.

Two basic cases are checked:

1.) Operators that take two instances of the same class/struct as arguments.

Example:

struct MyStruct {
  int x;  
  bool operator==(const MyStruct& rhs) const {return this->x == rhs.x; }
};

void testOperators(){
 MyStruct S;
 if(S == S) return; // Sides are equivalent.
}

2.) Overloaded operators that compare an instance of a class/struct with an integer constant expression.

Example:

struct S {
  S(){x = 1;}
  int x;
bool operator==(const int &i) const {return x == i;}
bool operator!=(int i) const {return x != i;}
};

void testOperators(){
  S s;
  if (s == 1 && s != 1) return; // Expression is always false.
}

It is checked, whether the arguments can be modified by the operator, filtering some of the dangerous side-effects.

Limitations:

• The checker is only able to recognize comparison ( ==, !=, >, <, >=, <= ) operators, and does not check for any other operator kind. This is because these operators are considered to be implemented to behave properly and conventionally in most cases.
• However, it is not checked whether the overloaded functions actually compare their arguments using the corresponding binary operator, or behave completely different from how one expects based on their name.

[Eugene.Zelenko]

clang-tidy/misc/RedundantExpressionCheck.cpp

58

Unnecessary empty line

151

Unnecessary empty line

583

Unnecessary empty line

604

Unnecessary empty line

1069

Unnecessary empty line

[barancsuk]

Address review comments: remove unnecessary lines from under function headers, if statements and from above else statements.

[xazax.hun]

clang-tidy/misc/RedundantExpressionCheck.cpp

150

Shouldn't you check in the body that you actually look at a copy constructor? Wouldn't other kinds of constructors interfere with your code? Or in case you want to skip constructors that acts as user defined conversions the function name or the documentation should reflect this.

512

Why do you need both the last two arguments?

517

You could do OperatorDecl->getType()->getAs<FunctionType>()->isConst() which is slightly shorter.

531

It looks like there is already a similar function: https://clang.llvm.org/doxygen/classclang_1_1BinaryOperator.html#ae394f72a1fce1435a47e79f816297329

[barancsuk]

Address review comments

clang-tidy/misc/RedundantExpressionCheck.cpp

150

The copy constructor is called when an instance of an object is passed by value to an operator. Since it is rather unlikely that an operator accepts values instead of references, I think there is no need to check for these cases in the current version.

512

You are right, I think ParamsToCheckCount is enough to set the count of the arguments that need checking. I also inserted an assert to ensure that non-existent parameters are not being accessed in the function.

[xazax.hun]

clang-tidy/misc/RedundantExpressionCheck.cpp

510

If the second parameter of this function only responsible for binary information (whether the second parameters should be checked), maybe it would be better to have the type bool and rename it accordingly.

511

Use unsigned rather than uint.

588

You could use OverloadedDecl here. Also, you should use dyn_cast since you later check whether the cast succeeded.

1074

Could use OverloadedDecl and use dyn_cast instead of cast here.

[barancsuk]

Address latest review comments

[xazax.hun]

clang-tidy/misc/RedundantExpressionCheck.cpp

337

Do you actually need this matcher? Can't you just use the same matcher as above instead of this one? They seem to do the same thing.

497

I would omit the Param from the name of this function, since it has nothing specific to the parameters.

1073

The result of the dyn_casts are now not checked. In case you think the cast will never fail, use cast and do not check the results. If the cast might fail, use dyn_cast and check the result.

1074

I woud move this check above the cast.

[barancsuk]

Address latest review comments

[xazax.hun]

clang-tidy/misc/RedundantExpressionCheck.cpp

590

You could use dyn_cast_or_null here and elide the null check above (and also even eliminate the OverloadedDecl variable).

1069

Do you need al these checks both here and in retrieveRelationalIntegerConstantExpr? Or is there a way to factor this out?

1076

You can use the same dyn_cast_or_null trick here as above.

[barancsuk]

Address latest review comments

clang-tidy/misc/RedundantExpressionCheck.cpp

1069

I think this can be easily eliminated, since these expressions are matched as overloaded operators with an operator kind that always accepts two arguments.

[xazax.hun]

Other than two nits it looks good to me. Wait one more reviewer just in case.

clang-tidy/misc/RedundantExpressionCheck.cpp

1067

The check is already done in retrieveRelationalIntegerConstantExpr as far as I understand. So in fact here you only check which diagnostic should be emitted based on the bindings from AST matchers.

1073

Do you need this check after retrieveRelationalIntegerConstantExpr is invoked above?

[barancsuk]

clang-tidy/misc/RedundantExpressionCheck.cpp

1073

In fact, retrieveRelationalIntegerConstantExpr is not invoked before this check.

The part of the code that calls retrieveRelationalIntegerConstantExpr, is the method checkRelationalExpr.
checkRelationalExpr is independent from this part of the code and even uses a distinct AST matcher.
It checks for redundant logical operator expressions with both operands being relational operator expressions, for example: (s1 == 1 && s1 != 1).

Here, overloaded operator expressions with two identical sides (e.g.: S == S) are checked.

These two cases are handled separately in the code, thus the method canOverloadedOperatorArgsBeModified is called twice at two different places: here and in retrieveRelationalIntegerConstantExpr.

[aaron.ballman]

clang-tidy/misc/RedundantExpressionCheck.cpp

512

For fun, can you add a test case where you have a malformed overloaded operator to ensure this assert does not trigger? e.g.,

struct S {
  bool operator==();
};

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

238

Spurious line removal.

252

Spurious line removal.

[barancsuk]

Address latest review comments

[barancsuk]

clang-tidy/misc/RedundantExpressionCheck.cpp

512

According to the C++ standard, 'it is not possible to change the number of operands of [overloaded] operators'.
Therefore I think the best solution here is to drop the assert itself, since it is impossible to define such a malformed overloaded operator (that compiles).

[aaron.ballman]

clang-tidy/misc/RedundantExpressionCheck.cpp

512

Ah, good point about that being ill-formed. Yeah, I think the assert can go away then. Thanks!

[aaron.ballman]

LGTM!

clang-tidy/misc/RedundantExpressionCheck.cpp

 namespace tidy {
 namespace misc {
 
 namespace {
 using llvm::APSInt;
 } // namespace
 
 static const llvm::StringSet<> KnownBannedMacroNames = {"EAGAIN", "EWOULDBLOCK",
                                                         "SIGCLD", "SIGCHLD"};
 
 static bool incrementWithoutOverflow(const APSInt &Value, APSInt &Result) {
   Result = Value;
   ++Result;
   return Value < Result;
 }
 
 static bool areEquivalentNameSpecifier(const NestedNameSpecifier *Left,
                                        const NestedNameSpecifier *Right) {
   llvm::FoldingSetNodeID LeftID, RightID;
   Left->Profile(LeftID);
   Right->Profile(RightID);
   return LeftID == RightID;
 }
 
 static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
   if (!Left || !Right)
     return !Left && !Right;
 
   Left = Left->IgnoreParens();
   Right = Right->IgnoreParens();
 
   // Compare classes.
   if (Left->getStmtClass() != Right->getStmtClass())
     return false;
static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
   case Stmt::FloatingLiteralClass:
     return cast<FloatingLiteral>(Left)->getValue().bitwiseIsEqual(
         cast<FloatingLiteral>(Right)->getValue());
 
   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())
       return false;
     return areEquivalentNameSpecifier(
         cast<DependentScopeDeclRefExpr>(Left)->getQualifier(),
         cast<DependentScopeDeclRefExpr>(Right)->getQualifier());
 
return cast<UnaryOperator>(Left)->getOpcode() ==
            cast<UnaryOperator>(Right)->getOpcode();
 
   case Stmt::BinaryOperatorClass:
     return cast<BinaryOperator>(Left)->getOpcode() ==
            cast<BinaryOperator>(Right)->getOpcode();
   }
 }
 
+// Returns the first subexpr of an expr that is not a cast or a copy
+// constructor's materialize expr.
+// Used by matcher `overloadedOperatorsArgumentsAreEquivalent` for stripping
+// unnecessary layers from arguments before deciding whether they are
+// equivalent.
+const Expr *ignoreCastsAndCopyConstructorCalls(const Expr *CallExpression) {
+  const Expr *NewExpr;
+
+  if (auto *ConstructorExpr = dyn_cast<CXXConstructExpr>(CallExpression)) {
+    if (ConstructorExpr->getNumArgs() == 1)
+      NewExpr = ConstructorExpr->getArg(0);
+  }
+
+  NewExpr = NewExpr->IgnoreCasts();
+  return NewExpr;
+}
+
 // For a given expression 'x', returns whether the ranges covered by the
 // relational operators are equivalent (i.e.  x <= 4 is equivalent to x < 5).
 static bool areEquivalentRanges(BinaryOperatorKind OpcodeLHS,
                                 const APSInt &ValueLHS,
                                 BinaryOperatorKind OpcodeRHS,
                                 const APSInt &ValueRHS) {
   assert(APSInt::compareValues(ValueLHS, ValueRHS) <= 0 &&
          "Values must be ordered");
AST_MATCHER(ConditionalOperator, expressionsAreEquivalent) {
   return areEquivalentExpr(Node.getTrueExpr(), Node.getFalseExpr());
 }
 
 AST_MATCHER(CallExpr, parametersAreEquivalent) {
   return Node.getNumArgs() == 2 &&
          areEquivalentExpr(Node.getArg(0), Node.getArg(1));
 }
 
+AST_MATCHER(CallExpr, overloadedOperatorsArgumentsAreEquivalent) {
+  return (
+      Node.getNumArgs() == 2 &&
+      areEquivalentExpr(ignoreCastsAndCopyConstructorCalls(Node.getArg(0)),
+                        ignoreCastsAndCopyConstructorCalls(Node.getArg(1))));
+}
+
 AST_MATCHER(BinaryOperator, binaryOperatorIsInMacro) {
   return Node.getOperatorLoc().isMacroID();
 }
 
 AST_MATCHER(ConditionalOperator, conditionalOperatorIsInMacro) {
   return Node.getQuestionLoc().isMacroID() || Node.getColonLoc().isMacroID();
 }
 
if (ConstExpr && ConstExpr->isIntegerConstantExpr(Value, *Result.Context))
     return true;
 
   return false;
 }
 
 // Overloaded `retrieveIntegerConstantExpr` for compatibility.
 static bool retrieveIntegerConstantExpr(const MatchFinder::MatchResult &Result,
                                         StringRef Id, APSInt &Value) {
-  const Expr* ConstExpr = nullptr;;
+  const Expr *ConstExpr = nullptr;
   return retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr);
 }
 
 // Returns a matcher for symbolic expressions (matches every expression except
 // ingeter constant expressions).
 static ast_matchers::internal::Matcher<Expr> matchSymbolicExpr(StringRef Id) {
   std::string SymId = (Id + "-sym").str();
   return ignoringParenImpCasts(

 }
 
 // Matches relational expressions: 'Expr <op> k' (i.e. x < 2, x != 3, 12 <= x).
 static ast_matchers::internal::Matcher<Expr>
 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),
       anyOf(allOf(hasLHS(matchSymbolicExpr(Id)),
                   hasRHS(matchIntegerConstantExpr(Id))),
             allOf(hasLHS(matchIntegerConstantExpr(Id)),
                   hasRHS(matchSymbolicExpr(Id)), expr().bind(SwapId)))));
 
matchRelationalIntegerConstantExpr(StringRef Id) {
 
   // Do not bind to double negation, it is uneffective.
   const auto NegateNegateRelationalExpr =
       unaryOperator(hasOperatorName("!"),
                     hasUnaryOperand(unaryOperator(
                         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
-// name 'Id'.
+// Checks whether a function param is non constant reference type, and may
+// be modified in the function.
+static bool isParamNonConstReferenceType(QualType ParamType) {
+  if (ParamType->isReferenceType() &&
+      !ParamType.getNonReferenceType().isConstQualified())
+    return true;
+  return false;
+}
+
+// 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,
+                                    int ParamCount, int ParamsToCheckCount) {
+  // 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 &&
+      !cast<FunctionType>(OperatorDecl->getType().getTypePtr())->isConst())
+    return true;
+
+  if (isParamNonConstReferenceType(OperatorDecl->getParamDecl(0)->getType()))
+    return true;
+
+  if ((ParamsToCheckCount == 2 && ParamCount == 2) &&
+      isParamNonConstReferenceType(OperatorDecl->getParamDecl(1)->getType()))
+    return true;
+
+  return false;
+}
+
+// OverloadedOperatorKind -> BinaryOperatorKind conversion.
+static bool convertOverloadedOpToBinOp(OverloadedOperatorKind OverloadedOp,
+                                       BinaryOperatorKind &BinaryOp) {
+  switch (OverloadedOp) {
+  case OO_EqualEqual:
+    BinaryOp = BO_EQ;
+    break;
+  case OO_ExclaimEqual:
+    BinaryOp = BO_NE;
+    break;
+  case OO_LessEqual:
+    BinaryOp = BO_LE;
+    break;
+  case OO_GreaterEqual:
+    BinaryOp = BO_GE;
+    break;
+  case OO_Less:
+    BinaryOp = BO_LT;
+    break;
+  case OO_Greater:
+    BinaryOp = BO_GT;
+    break;
+  default:
+    return false;
+  }
+  return true;
+}
+
+// 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,
     APSInt &Value, const Expr *&ConstExpr) {
   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.
     Opcode = Bin->getOpcode();
     OperandExpr = Bin;
 
     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 =
+        cast<FunctionDecl>(OverloadedOperatorExpr->getCalleeDecl());
+    if (!OverloadedFunctionDecl || OverloadedOperatorExpr->getNumArgs() != 2)
+      return false;
+
+    if (canOverloadedOperatorArgsBeModified(
+            OverloadedFunctionDecl, OverloadedFunctionDecl->getNumParams(), 1))
+      return false;
+
+    if (!OverloadedOperatorExpr->getArg(1)->isIntegerConstantExpr(
+            Value, *Result.Context))
+      return false;
+
+    Symbol =
+        ignoreCastsAndCopyConstructorCalls(OverloadedOperatorExpr->getArg(0));
+
+    OperandExpr = OverloadedOperatorExpr;
+
+    return convertOverloadedOpToBinOp(OverloadedOperatorExpr->getOperator(),
+                                      Opcode);
   } else {
     return false;
   }
 
   if (!retrieveSymbolicExpr(Result, Id, Symbol))
     return false;
 
   if (Result.Nodes.getNodeAs<Expr>(SwapId))
     Opcode = BinaryOperator::reverseComparisonOp(Opcode);
   if (Result.Nodes.getNodeAs<Expr>(NegateId))
     Opcode = BinaryOperator::negateComparisonOp(Opcode);
+
   return true;
 }
 
 // Checks for expressions like (X == 4) && (Y != 9)
-static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp, const ASTContext *AstCtx) {
+static bool areSidesBinaryConstExpressions(const BinaryOperator *&BinOp,
+                                           const ASTContext *AstCtx) {
   if (!isa<BinaryOperator>(BinOp->getLHS()) ||
       !isa<BinaryOperator>(BinOp->getRHS()))
     return false;
-
-  BinaryOperator* LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS());
-  BinaryOperator* RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS());
-
+  BinaryOperator *LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS());
+  BinaryOperator *RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS());
   if ((LhsBinOp->getLHS()->isIntegerConstantExpr(*AstCtx) ||
        LhsBinOp->getRHS()->isIntegerConstantExpr(*AstCtx)) &&
       (RhsBinOp->getLHS()->isIntegerConstantExpr(*AstCtx) ||
        RhsBinOp->getRHS()->isIntegerConstantExpr(*AstCtx)))
     return true;
   return false;
 }
 
static bool areExprsFromDifferentMacros(const Expr *LhsExpr,
   StringRef LhsMacroName = Lexer::getImmediateMacroName(LhsLoc, SM, LO);
   StringRef RhsMacroName = Lexer::getImmediateMacroName(RhsLoc, SM, LO);
 
   if (LhsMacroName == RhsMacroName)
     return false;
   return true;
 }
 
-static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr, const Expr *&RhsExpr) {
+static bool areExprsMacroAndNonMacro(const Expr *&LhsExpr,
+                                     const Expr *&RhsExpr) {
   if (!LhsExpr || !RhsExpr)
     return false;
 
   SourceLocation LhsLoc = LhsExpr->getExprLoc();
   SourceLocation RhsLoc = RhsExpr->getExprLoc();
 
   if ((LhsLoc.isMacroID() && !RhsLoc.isMacroID()) ||
       (!LhsLoc.isMacroID() && RhsLoc.isMacroID()))

 
 // Checks whether a set of operators are able to create a meaningful and
 // non-redundant expression.
 // The function is used by `isIntendedMacroExpression` to filter operator sets
 // like (<, &&, <=), that always introduce redundant expressions.
 static bool isOperatorSetMeaningful(BinaryOperatorKind &Opcode,
                                     BinaryOperatorKind &LhsOpcode,
                                     BinaryOperatorKind &RhsOpcode) {
-
   if (!BinaryOperator::isLogicalOp(Opcode))
     return false;
 
   if ((isLessOperator(LhsOpcode) && isGreaterOperator(RhsOpcode)) ||
       (isLessOperator(RhsOpcode) && isGreaterOperator(LhsOpcode)))
     return true;
 
   if (Opcode == BO_LAnd) {
if ((areExprsFromDifferentMacros(LhsExpr, RhsExpr, AstCtx) ||
     return true;
   return false;
 }
 
 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(
       binaryOperator(anyOf(hasOperatorName("-"), hasOperatorName("/"),
                            hasOperatorName("%"), hasOperatorName("|"),
                            hasOperatorName("&"), hasOperatorName("^"),
                            matchers::isComparisonOperator(),
                            hasOperatorName("&&"), hasOperatorName("||"),
                            hasOperatorName("=")),
                      operandsAreEquivalent(),
                      // Filter noisy false positives.
                      unless(isInTemplateInstantiation()),
                      unless(binaryOperatorIsInMacro()),
                      unless(hasType(realFloatingPointType())),
                      unless(hasEitherOperand(hasType(realFloatingPointType()))),
                      unless(hasLHS(AnyLiteralExpr)),
                      unless(hasDescendant(BannedIntegerLiteral)))
           .bind("binary"),
       this);
 
   // Conditional (trenary) operator with equivalent operands, like (Y ? X : X).
-  Finder->addMatcher(
-      conditionalOperator(expressionsAreEquivalent(),
+  Finder->addMatcher(conditionalOperator(expressionsAreEquivalent(),
                                          // Filter noisy false positives.
                                          unless(conditionalOperatorIsInMacro()),
                                          unless(isInTemplateInstantiation()))
                          .bind("cond"),
                      this);
 
   // Overloaded operators with equivalent operands.
   Finder->addMatcher(
       cxxOperatorCallExpr(
           anyOf(
               hasOverloadedOperatorName("-"), hasOverloadedOperatorName("/"),
               hasOverloadedOperatorName("%"), hasOverloadedOperatorName("|"),
               hasOverloadedOperatorName("&"), hasOverloadedOperatorName("^"),
               hasOverloadedOperatorName("=="), hasOverloadedOperatorName("!="),
               hasOverloadedOperatorName("<"), hasOverloadedOperatorName("<="),
               hasOverloadedOperatorName(">"), hasOverloadedOperatorName(">="),
               hasOverloadedOperatorName("&&"), hasOverloadedOperatorName("||"),
               hasOverloadedOperatorName("=")),
-          parametersAreEquivalent(),
+          overloadedOperatorsArgumentsAreEquivalent(),
           // Filter noisy false positives.
           unless(isMacro()), unless(isInTemplateInstantiation()))
           .bind("call"),
       this);
 
   // Match common expressions and apply more checks to find redundant
   // sub-expressions.
   //   a) Expr <op> K1 == K2
if (const auto *ComparisonOperator = Result.Nodes.getNodeAs<BinaryOperator>(
     // Bring expr to a canonical form: smallest constant must be on the left.
     if (APSInt::compareValues(LhsValue, RhsValue) > 0) {
       std::swap(LhsExpr, RhsExpr);
       std::swap(LhsValue, RhsValue);
       std::swap(LhsSymbol, RhsSymbol);
       std::swap(LhsOpcode, RhsOpcode);
     }
 
+    // Do not warn if macro expr can be considered intentional.
     if (isIntendedMacroExpression(Opcode, LhsOpcode, LhsConst, RhsOpcode,
                                   RhsConst, Result.Context))
       return;
 
     if ((Opcode == BO_LAnd || Opcode == BO_LOr) &&
         areEquivalentRanges(LhsOpcode, LhsValue, RhsOpcode, RhsValue)) {
       diag(ComparisonOperator->getOperatorLoc(),
            "equivalent expression on both sides of logical operator");
if (Opcode == BO_LOr) {
         diag(LhsExpr->getExprLoc(), "expression is redundant");
       }
     }
   }
 }
 
 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,
-                                     RhsConst, Result.Context))
-          return;
+      if (!retrieveConstExprFromBothSides(BinOp, MainOpcode, SideOpcode,
+                                          LhsConst, RhsConst, Result.Context))
 
       if (isIntendedMacroExpression(MainOpcode, SideOpcode, LhsConst,
                                     SideOpcode, RhsConst, Result.Context))
         return;
     }
 
     diag(BinOp->getOperatorLoc(), "both sides of operator are equivalent");
   }
   if (const auto *CondOp =
           Result.Nodes.getNodeAs<ConditionalOperator>("cond")) {
-
     const auto *TrueExpr = CondOp->getTrueExpr();
     const auto *FalseExpr = CondOp->getFalseExpr();
 
     if (areExprsFromDifferentMacros(TrueExpr, FalseExpr, Result.Context) ||
         areExprsMacroAndNonMacro(TrueExpr, FalseExpr))
       return;
     diag(CondOp->getColonLoc(),
          "'true' and 'false' expressions are equivalent");
   }
 
+  // Check overloaded operators with equivalent operands.
   if (const auto *Call = Result.Nodes.getNodeAs<CXXOperatorCallExpr>("call")) {
+    const auto *OverloadedFunctionDecl =
+        cast<FunctionDecl>(Call->getCalleeDecl());
+
+    if (!OverloadedFunctionDecl || Call->getNumArgs() != 2)
+      return;
+
+    if (canOverloadedOperatorArgsBeModified(
+            OverloadedFunctionDecl, OverloadedFunctionDecl->getNumParams(), 2))
+      return;
+
     diag(Call->getOperatorLoc(),
          "both sides of overloaded operator are equivalent");
   }
 
   // Check for the following binded expressions:
   // - "binop-const-compare-to-sym",
   // - "binop-const-compare-to-binop-const",
   // Produced message:

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

int TestConditional(int x, int y) {
   k += (y < 0) ? COND_OP_MACRO : 9;
   // Do not match for conditional operators with expressions from different macros.
   k += (y < 0) ? COND_OP_MACRO : COND_OP_OTHER_MACRO;
   return k;
 }
 #undef COND_OP_MACRO
 #undef COND_OP_OTHER_MACRO
 
+// Overloaded operators comparing 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>=(MyStruct rhs) const { rhs.x++; 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, MyStruct rhs) { rhs.x++; return lhs.x < rhs.x; } // not modifing
+bool operator>(const MyStruct& lhs, MyStruct& rhs) { 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))
 #define COND(x, y, z) ((x)?(y):(z))
 #define EQUALS(x, y) (x) == (y)
 
 int TestMacro(int X, int Y) {
   LT(0, 0);

 
 struct MyClass {
 static const int Value = 42;
 };
 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>(); }
 
 int TestArithmetic(int X, int Y) {
   if (X + 1 == X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
   if (X + 1 != X) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
int TestArithmetic(int X, int Y) {
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always false
   if (X + 1 != X - -2) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
 
   if (X + 1 == X - (~0)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
   if (X + 1 == X - (~0U)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
-
   if (X + 1 == X - (~0ULL)) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: logical expression is always true
 
   // Should not match.
   if (X + 0.5 == X) return 1;
   if (X + 1 == Y) return 1;
   if (X + 1 == Y + 1) return 1;
   if (X + 1 == Y + 2) return 1;
 
   return 0;
 }
 
 int TestBitwise(int X, int Y) {
-
   if ((X & 0xFF) == 0xF00) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
   if ((X & 0xFF) != 0xF00) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
   if ((X | 0xFF) == 0xF00) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
   if ((X | 0xFF) != 0xF00) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always true
int TestBitwise(int X, int Y) {
   if ((0xFFLL & X) == 0xF00) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:20: warning: logical expression is always false
   if ((0xFF & X) == 0xF00ULL) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: logical expression is always false
 
   return 0;
 }
 
+// Overloaded operators comparing an instance of a struct and an integer constant.
+// Overloaded comparison operators without any possible side effect.
+struct S {
+  S(){x = 1;}
+  int x;
+bool operator==(const int &i) const {return x == i;}
+bool operator!=(int i) const {return x != i;}
+bool operator>(const int& i) const {return x > i;}
+bool operator<(int i) const {return x < i;}
+};
+
+bool operator<=(const S &s, int i) {return s.x <= i;}
+bool operator>=(S s, const int &i) {s.x--; return s.x >= i;}
+
+// Overloaded comparison operators that are able to modify their params,
+// therefore may have side effects.
+struct S2 {
+  S2(){x = 1;}
+  int x;
+  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
 #undef CONFIG
 #define CONFIG 1
   if (CONFIG || X) return 1;
 #undef CONFIG
 
#undef CONFIG
   // Should not match.
   if (X == 10 && Y == 10) return 1;
   if (X != 10 && X != 12) return 1;
   if (X == 10 || X == 12) return 1;
   if (!X && !Y) return 1;
   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) {
   if (X == 10 && X > 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
   if (X == 10 && X < 10) return 1;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
   if (X < 10 && X > 10) return 1;