[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

142

Unnecessary empty line

515

Unnecessary empty line

536

Unnecessary empty line

920

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

141

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.

475

Why do you need both the last two arguments?

480

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

494

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

141

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.

475

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

473

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.

474

Use unsigned rather than uint.

520

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

925

Could use OverloadedDecl and use dyn_cast instead of cast here.

[barancsuk]

Address latest review comments

[xazax.hun]

clang-tidy/misc/RedundantExpressionCheck.cpp

311

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.

460

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

924

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.

925

I woud move this check above the cast.

[barancsuk]

Address latest review comments

[xazax.hun]

clang-tidy/misc/RedundantExpressionCheck.cpp

522

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

920

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

927

You can use the same dyn_cast_or_null trick here as above.

[barancsuk]

Address latest review comments

clang-tidy/misc/RedundantExpressionCheck.cpp

920

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

919

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.

924

Do you need this check after retrieveRelationalIntegerConstantExpr is invoked above?

[barancsuk]

clang-tidy/misc/RedundantExpressionCheck.cpp

924

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

475

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

475

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

475

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

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;
return LeftLit.getBitWidth() == RightLit.getBitWidth() &&
            LeftLit == RightLit;
   }
   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());
   case Stmt::DeclRefExprClass:
static bool areEquivalentExpr(const Expr *Left, const Expr *Right) {
   }
 }
 
 // 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");
   // Handle the case where constants are the same: x <= 4  <==>  x <= 4.
   if (APSInt::compareValues(ValueLHS, ValueRHS) == 0)
     return OpcodeLHS == OpcodeRHS;
 
   // Handle the case where constants are off by one: x <= 4  <==>  x < 5.
   APSInt ValueLHS_plus1;
   return ((OpcodeLHS == BO_LE && OpcodeRHS == BO_LT) ||
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(BinaryOperator, binaryOperatorIsInMacro) {
   return Node.getOperatorLoc().isMacroID();
 }
 
 AST_MATCHER(ConditionalOperator, conditionalOperatorIsInMacro) {
   return Node.getQuestionLoc().isMacroID() || Node.getColonLoc().isMacroID();
 }
 
 AST_MATCHER(Expr, isMacro) { return Node.getExprLoc().isMacroID(); }

 }
 
 // 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.
   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 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,
     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 = 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;
   }
 
   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;
 }
 
static bool areExprsFromDifferentMacros(const Expr *LhsExpr,
 
   const SourceManager &SM = AstCtx->getSourceManager();
   const LangOptions &LO = AstCtx->getLangOpts();
 
   return !(Lexer::getImmediateMacroName(LhsLoc, SM, LO) ==
           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;
 
   SourceLocation LhsLoc = LhsExpr->getExprLoc();
   SourceLocation RhsLoc = RhsExpr->getExprLoc();
 
   return LhsLoc.isMacroID() != RhsLoc.isMacroID();
 }
 
 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("^"),
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 (areExprsFromDifferentMacros(LhsConst, RhsConst, Result.Context) ||
           areExprsMacroAndNonMacro(LhsConst, RhsConst))
         return;
     }
 
     diag(BinOp->getOperatorLoc(), "both sides of operator are equivalent");
   }
 
   if (const auto *CondOp =
           Result.Nodes.getNodeAs<ConditionalOperator>("cond")) {
     const Expr *TrueExpr = CondOp->getTrueExpr();
     const Expr *FalseExpr = CondOp->getFalseExpr();
 
     if (areExprsFromDifferentMacros(TrueExpr, FalseExpr, Result.Context) ||
         areExprsMacroAndNonMacro(TrueExpr, FalseExpr))
       return;
     diag(CondOp->getColonLoc(),
          "'true' and 'false' expressions are equivalent");
   }
 
   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");
   }
 
   // Check for the following bound expressions:
   // - "binop-const-compare-to-sym",
   // - "binop-const-compare-to-binop-const",
   // Produced message:
   // -> "logical expression is always false/true"

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 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))
 #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 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;
 #undef CONFIG
 
   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;
#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;