[PATCH] D122111: [clang-tidy] Fix false positives in `misc-redundant-expression` check

[Fabian Wolff via Phabricator via cfe-commits]

fwolff created this revision.
fwolff added reviewers: alexfh, aaron.ballman, pavelkryukov.
fwolff added a project: clang-tools-extra.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
fwolff requested review of this revision.
Herald added a subscriber: cfe-commits.

It turns out that the right-hand sides of overloaded comparison operators are 
currently not checked at all in `misc-redundant-expression`, leading to such 
false positives as given in #54011 
<https://github.com/llvm/llvm-project/issues/54011> or here 
<https://godbolt.org/z/aYxY343Kn>. This patch fixes this.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D122111

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


Index: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
@@ -453,6 +453,11 @@
   if (s1 >= 1 || s1 <= 1) return true;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always 
true
 
+  // 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;
   if (s2 == 1 || s2 != 1) return true;
Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -569,6 +569,7 @@
   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,8 @@
       cxxOperatorCallExpr(
           hasAnyOverloadedOperatorName("==", "!=", "<", "<=", ">", ">="),
           // Filter noisy false positives.
-          unless(isMacro()), unless(isInTemplateInstantiation()))
+          unless(isMacro()), unless(isInTemplateInstantiation()),
+          hasRHS(ignoringParenImpCasts(integerLiteral().bind(ConstId))))
           .bind(OverloadId);
 
   return anyOf(RelationalExpr, CastExpr, NegateRelationalExpr,
@@ -683,6 +685,9 @@
     OperandExpr = OverloadedOperatorExpr;
     Opcode = 
BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator());
 
+    if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr))
+      return false;
+
     return BinaryOperator::isComparisonOp(Opcode);
   } else {
     return false;



Index: clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp
@@ -453,6 +453,11 @@
   if (s1 >= 1 || s1 <= 1) return true;
   // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: logical expression is always true
 
+  // 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;
   if (s2 == 1 || s2 != 1) return true;
Index: clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp
@@ -569,6 +569,7 @@
   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,8 @@
       cxxOperatorCallExpr(
           hasAnyOverloadedOperatorName("==", "!=", "<", "<=", ">", ">="),
           // Filter noisy false positives.
-          unless(isMacro()), unless(isInTemplateInstantiation()))
+          unless(isMacro()), unless(isInTemplateInstantiation()),
+          hasRHS(ignoringParenImpCasts(integerLiteral().bind(ConstId))))
           .bind(OverloadId);
 
   return anyOf(RelationalExpr, CastExpr, NegateRelationalExpr,
@@ -683,6 +685,9 @@
     OperandExpr = OverloadedOperatorExpr;
     Opcode = BinaryOperator::getOverloadedOpcode(OverloadedOperatorExpr->getOperator());
 
+    if (!retrieveIntegerConstantExpr(Result, Id, Value, ConstExpr))
+      return false;
+
     return BinaryOperator::isComparisonOp(Opcode);
   } else {
     return false;

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits