[clang-tidy] Fix false positives in misc-redundant-expression check

Reviewed By: aaron.ballman

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

Author: FabianWolff

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

@@ -569,6 +569,7 @@ matchRelationalIntegerConstantExpr(StringRef Id) {
   std::string SwapId = (Id + "-swap").str();
   std::string NegateId = (Id + "-negate").str();
   std::string OverloadId = (Id + "-overload").str();
+  std::string ConstId = (Id + "-const").str();
 
   const auto RelationalExpr = ignoringParenImpCasts(binaryOperator(
       isComparisonOperator(), expr().bind(Id),
@@ -600,7 +601,9 @@ matchRelationalIntegerConstantExpr(StringRef Id) {
       cxxOperatorCallExpr(
           hasAnyOverloadedOperatorName("==", "!=", "<", "<=", ">", ">="),
           // Filter noisy false positives.
-          unless(isMacro()), unless(isInTemplateInstantiation()))
+          unless(isMacro()), unless(isInTemplateInstantiation()),
+          anyOf(hasLHS(ignoringParenImpCasts(integerLiteral().bind(ConstId))),
+                hasRHS(ignoringParenImpCasts(integerLiteral().bind(ConstId)))))
           .bind(OverloadId);
 
   return anyOf(RelationalExpr, CastExpr, NegateRelationalExpr,
@@ -674,16 +677,38 @@ static bool retrieveRelationalIntegerConstantExpr(
     if (canOverloadedOperatorArgsBeModified(OverloadedOperatorExpr, false))
       return false;
 
+    bool IntegerConstantIsFirstArg = false;
+
     if (const auto *Arg = OverloadedOperatorExpr->getArg(1)) {
       if (!Arg->isValueDependent() &&
-          !Arg->isIntegerConstantExpr(*Result.Context))
-        return false;
-    }
-    Symbol = OverloadedOperatorExpr->getArg(0);
+          !Arg->isIntegerConstantExpr(*Result.Context)) {
+        IntegerConstantIsFirstArg = true;
+        if (const auto *Arg = OverloadedOperatorExpr->getArg(0)) {
+          if (!Arg->isValueDependent() &&
+              !Arg->isIntegerConstantExpr(*Result.Context))
+            return false;
+        } else
+          return false;
+      }
+    } else
+      return false;
+
+    Symbol = OverloadedOperatorExpr->getArg(IntegerConstantIsFirstArg ? 1 : 0);
     OperandExpr = OverloadedOperatorExpr;
     Opcode = BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator());
 
-    return BinaryOperator::isComparisonOp(Opcode);
+    if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr))
+      return false;
+
+    if (!BinaryOperator::isComparisonOp(Opcode))
+      return false;
+
+    // The call site of this function expects the constant on the RHS,
+    // so change the opcode accordingly.
+    if (IntegerConstantIsFirstArg)
+      Opcode = BinaryOperator::reverseComparisonOp(Opcode);
+
+    return true;
   } else {
     return false;
   }

clang-tools-extra/docs/ReleaseNotes.rst

@@ -116,6 +116,9 @@ Changes in existing checks
   <clang-tidy/checks/readability-const-return-type>` when a pure virtual function
   overrided has a const return type. Removed the fix for a virtual function.
 
+- Fixed a false positive in :doc:`misc-redundant-expression <clang-tidy/checks/misc-redundant-expression>`
+  involving overloaded comparison operators.
+
 Removed checks
 ^^^^^^^^^^^^^^
 

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

@@ -369,6 +369,11 @@ struct S {
 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
 
+bool operator==(int i, const S &s) { return s == i; } // not modifying
+bool operator<(const int &i, const S &s) { return s > i; } // not modifying
+bool operator<=(const int &i, const S &s) { return s >= i; } // not modifying
+bool operator>(const int &i, const S &s) { return s < i; } // not modifying
+
 struct S2 {
   S2() { x = 1; }
   int x;
@@ -452,6 +457,25 @@ int TestLogical(int X, int Y){
   // 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
+  if (s1 >= 2 && s1 <= 0) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always false
+
+  // Same test as above but with swapped LHS/RHS on one side of the logical operator.
+  if (1 == s1 && s1 == 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: equivalent expression on both sides of logical operator
+  if (1 == s1 || s1 != 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+  if (1 < s1 && s1 < 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+  if (1 <= s1 || s1 <= 1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
+  if (2 < s1 && 0 > s1) return true;
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: logical expression is always false
+
+  // Test for absence of false positives (issue #54011).
+  if (s1 == 1 || s1 == 2) return true;
+  if (s1 > 1 && s1 < 3) return true;
+  if (s1 >= 2 || s1 <= 0) return true;
 
   // Test for overloaded operators that may modify their params.
   S2 s2;