From 07675b0b38e930c2c582122ac93efcdf7859a772 Mon Sep 17 00:00:00 2001 From: Fabian Wolff Date: Wed, 23 Mar 2022 00:28:18 +0100 Subject: [PATCH] [clang-tidy] Fix false positives in `misc-redundant-expression` check Reviewed By: aaron.ballman Differential Revision: https://reviews.llvm.org/D122111 --- .../clang-tidy/misc/RedundantExpressionCheck.cpp | 37 ++++++++++++++++++---- clang-tools-extra/docs/ReleaseNotes.rst | 3 ++ .../checkers/misc-redundant-expression.cpp | 24 ++++++++++++++ 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp b/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp index f8073bf..e683843 100644 --- a/clang-tools-extra/clang-tidy/misc/RedundantExpressionCheck.cpp +++ b/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; } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 551e36d..9e68fb1 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -116,6 +116,9 @@ Changes in existing checks ` 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 ` + involving overloaded comparison operators. + Removed checks ^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp b/clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp index 075c3ac..31fe8e0 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/misc-redundant-expression.cpp +++ b/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; -- 2.7.4