From 9769afe434650fb5c75e0bbf0dbcd62d1f95ce74 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Fri, 25 Jun 2021 16:25:14 +0200 Subject: [PATCH] knownConditionTrueFalse; avoid several warnings when nonzero expression is compared to see if it is positive or negative --- lib/checkcondition.cpp | 11 ++++++ lib/checkother.cpp | 88 ++++++++++++++++++++++++++++-------------- lib/checkother.h | 7 ++++ test/testcondition.cpp | 15 +++++-- 4 files changed, 87 insertions(+), 34 deletions(-) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index ec3590fcb..3f2bf0604 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -29,6 +29,8 @@ #include "tokenize.h" #include "valueflow.h" +#include "checkother.h" // comparisonNonZeroExpressionLessThanZero and testIfNonZeroExpressionIsPositive + #include #include #include @@ -1427,6 +1429,15 @@ void CheckCondition::alwaysTrueFalse() if (isConstVarExpression(tok, "[|(|&|+|-|*|/|%|^|>>|<<")) continue; + // there are specific warnings about nonzero expressions (pointer/unsigned) + // do not write alwaysTrueFalse for these comparisons. + { + const ValueFlow::Value *zeroValue = nullptr; + const Token *nonZeroExpr = nullptr; + if (CheckOther::comparisonNonZeroExpressionLessThanZero(tok, &zeroValue, &nonZeroExpr) || CheckOther::testIfNonZeroExpressionIsPositive(tok, &zeroValue, &nonZeroExpr)) + continue; + } + const bool constIfWhileExpression = tok->astParent() && Token::Match(tok->astTop()->astOperand1(), "if|while") && !tok->astTop()->astOperand1()->isConstexpr() && (Token::Match(tok->astParent(), "%oror%|&&") || Token::Match(tok->astParent()->astOperand1(), "if|while")); diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 49362ce4d..e0e8c07d1 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2340,41 +2340,69 @@ void CheckOther::checkSignOfUnsignedVariable() for (const Scope * scope : symbolDatabase->functionScopes) { // check all the code in the function for (const Token *tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { - if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2()) - continue; - - const ValueFlow::Value *v1 = tok->astOperand1()->getValue(0); - const ValueFlow::Value *v2 = tok->astOperand2()->getValue(0); - - if (Token::Match(tok, "<|<=") && v2 && v2->isKnown()) { - const ValueType* vt = tok->astOperand1()->valueType(); - if (vt && vt->pointer) - pointerLessThanZeroError(tok, v2); - if (vt && vt->sign == ValueType::UNSIGNED) - unsignedLessThanZeroError(tok, v2, tok->astOperand1()->expressionString()); - } else if (Token::Match(tok, ">|>=") && v1 && v1->isKnown()) { - const ValueType* vt = tok->astOperand2()->valueType(); - if (vt && vt->pointer) - pointerLessThanZeroError(tok, v1); - if (vt && vt->sign == ValueType::UNSIGNED) - unsignedLessThanZeroError(tok, v1, tok->astOperand2()->expressionString()); - } else if (Token::simpleMatch(tok, ">=") && v2 && v2->isKnown()) { - const ValueType* vt = tok->astOperand1()->valueType(); - if (vt && vt->pointer) - pointerPositiveError(tok, v2); - if (vt && vt->sign == ValueType::UNSIGNED) - unsignedPositiveError(tok, v2, tok->astOperand1()->expressionString()); - } else if (Token::simpleMatch(tok, "<=") && v1 && v1->isKnown()) { - const ValueType* vt = tok->astOperand2()->valueType(); - if (vt && vt->pointer) - pointerPositiveError(tok, v1); - if (vt && vt->sign == ValueType::UNSIGNED) - unsignedPositiveError(tok, v1, tok->astOperand2()->expressionString()); + const ValueFlow::Value *zeroValue = nullptr; + const Token *nonZeroExpr = nullptr; + if (comparisonNonZeroExpressionLessThanZero(tok, &zeroValue, &nonZeroExpr)) { + const ValueType* vt = nonZeroExpr->valueType(); + if (vt->pointer) + pointerLessThanZeroError(tok, zeroValue); + else + unsignedLessThanZeroError(tok, zeroValue, nonZeroExpr->expressionString()); + } else if (testIfNonZeroExpressionIsPositive(tok, &zeroValue, &nonZeroExpr)) { + const ValueType* vt = nonZeroExpr->valueType(); + if (vt->pointer) + pointerPositiveError(tok, zeroValue); + else + unsignedPositiveError(tok, zeroValue, nonZeroExpr->expressionString()); } } } } +bool CheckOther::comparisonNonZeroExpressionLessThanZero(const Token *tok, const ValueFlow::Value **zeroValue, const Token **nonZeroExpr) +{ + if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2()) + return false; + + const ValueFlow::Value *v1 = tok->astOperand1()->getValue(0); + const ValueFlow::Value *v2 = tok->astOperand2()->getValue(0); + + if (Token::Match(tok, "<|<=") && v2 && v2->isKnown()) { + *zeroValue = v2; + *nonZeroExpr = tok->astOperand1(); + } else if (Token::Match(tok, ">|>=") && v1 && v1->isKnown()) { + *zeroValue = v1; + *nonZeroExpr = tok->astOperand2(); + } else { + return false; + } + + const ValueType* vt = (*nonZeroExpr)->valueType(); + return vt && (vt->pointer || vt->sign == ValueType::UNSIGNED); +} + +bool CheckOther::testIfNonZeroExpressionIsPositive(const Token *tok, const ValueFlow::Value **zeroValue, const Token **nonZeroExpr) +{ + if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2()) + return false; + + const ValueFlow::Value *v1 = tok->astOperand1()->getValue(0); + const ValueFlow::Value *v2 = tok->astOperand2()->getValue(0); + + if (Token::simpleMatch(tok, ">=") && v2 && v2->isKnown()) { + *zeroValue = v2; + *nonZeroExpr = tok->astOperand1(); + } else if (Token::simpleMatch(tok, "<=") && v1 && v1->isKnown()) { + *zeroValue = v1; + *nonZeroExpr = tok->astOperand2(); + } else { + return false; + } + + const ValueType* vt = (*nonZeroExpr)->valueType(); + return vt && (vt->pointer || vt->sign == ValueType::UNSIGNED); +} + void CheckOther::unsignedLessThanZeroError(const Token *tok, const ValueFlow::Value * v, const std::string &varname) { reportError(getErrorPath(tok, v, "Unsigned less than zero"), Severity::style, "unsignedLessThanZero", diff --git a/lib/checkother.h b/lib/checkother.h index 827cd176b..215ad8bff 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -104,6 +104,13 @@ public: checkOther.checkModuloOfOne(); } + /** Is expression a comparison that checks if a nonzero (unsigned/pointer) expression is less than zero? */ + static bool comparisonNonZeroExpressionLessThanZero(const Token *tok, const ValueFlow::Value **zeroValue, const Token **nonZeroExpr); + + /** Is expression a comparison that checks if a nonzero (unsigned/pointer) expression is positive? */ + static bool testIfNonZeroExpressionIsPositive(const Token *tok, const ValueFlow::Value **zeroValue, const Token **nonZeroExpr); + + /** @brief Clarify calculation for ".. a * b ? .." */ void clarifyCalculation(); diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 11d2e292f..cf2a65f80 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -2375,8 +2375,8 @@ private: check("void f1(const std::string &s) { if(s.size() > 0) if(s.empty()) {}}"); ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str()); - check("void f1(const std::string &s) { if(s.size() < 0) if(s.empty()) {}} "); - ASSERT_EQUALS("[test.cpp:1]: (style) Condition 's.size()<0' is always false\n", errout.str()); + check("void f1(const std::string &s) { if(s.size() < 0) if(s.empty()) {}} "); // <- CheckOther reports: checking if unsigned expression is less than zero + ASSERT_EQUALS("", errout.str()); check("void f1(const std::string &s) { if(s.empty()) if(s.size() > 42) {}}"); ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str()); @@ -2411,8 +2411,8 @@ private: check("void f1(const std::string &s) { if(s.size() < 2) if(s.empty()) {}}"); ASSERT_EQUALS("", errout.str()); - check("void f1(const std::string &s) { if(s.size() >= 0) if(s.empty()) {}} "); - ASSERT_EQUALS("[test.cpp:1]: (style) Condition 's.size()>=0' is always true\n", errout.str()); + check("void f1(const std::string &s) { if(s.size() >= 0) if(s.empty()) {}} "); // CheckOther says: Unsigned expression 's.size()' can't be negative so it is unnecessary to test it. [unsignedPositive] + ASSERT_EQUALS("", errout.str()); // TODO: These are identical condition since size cannot be negative check("void f1(const std::string &s) { if(s.size() <= 0) if(s.empty()) {}}"); @@ -3632,6 +3632,13 @@ private: " if (x == -1) {}\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + // do not report both unsignedLessThanZero and knownConditionTrueFalse + check("void foo(unsigned int max) {\n" + " unsigned int num = max - 1;\n" + " if (num < 0) {}\n" // <- do not report knownConditionTrueFalse + "}"); + ASSERT_EQUALS("", errout.str()); } void alwaysTrueInfer() {