Skip to content

Commit 9769afe

Browse files
danmardanmar
committed
knownConditionTrueFalse; avoid several warnings when nonzero expression is compared to see if it is positive or negative
1 parent 452c924 commit 9769afe

4 files changed

Lines changed: 87 additions & 34 deletions

File tree

‎lib/checkcondition.cpp‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@
2929
#include "tokenize.h"
3030
#include "valueflow.h"
3131

32+
#include "checkother.h" // comparisonNonZeroExpressionLessThanZero and testIfNonZeroExpressionIsPositive
33+
3234
#include <algorithm>
3335
#include <limits>
3436
#include <list>
@@ -1427,6 +1429,15 @@ void CheckCondition::alwaysTrueFalse()
14271429
if (isConstVarExpression(tok, "[|(|&|+|-|*|/|%|^|>>|<<"))
14281430
continue;
14291431

1432+
// there are specific warnings about nonzero expressions (pointer/unsigned)
1433+
// do not write alwaysTrueFalse for these comparisons.
1434+
{
1435+
const ValueFlow::Value *zeroValue = nullptr;
1436+
const Token *nonZeroExpr = nullptr;
1437+
if (CheckOther::comparisonNonZeroExpressionLessThanZero(tok, &zeroValue, &nonZeroExpr) || CheckOther::testIfNonZeroExpressionIsPositive(tok, &zeroValue, &nonZeroExpr))
1438+
continue;
1439+
}
1440+
14301441
const bool constIfWhileExpression =
14311442
tok->astParent() && Token::Match(tok->astTop()->astOperand1(), "if|while") && !tok->astTop()->astOperand1()->isConstexpr() &&
14321443
(Token::Match(tok->astParent(), "%oror%|&&") || Token::Match(tok->astParent()->astOperand1(), "if|while"));

‎lib/checkother.cpp‎

Lines changed: 58 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -2340,41 +2340,69 @@ void CheckOther::checkSignOfUnsignedVariable()
23402340
for (const Scope * scope : symbolDatabase->functionScopes) {
23412341
// check all the code in the function
23422342
for (const Token *tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) {
2343-
if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2())
2344-
continue;
2345-
2346-
const ValueFlow::Value *v1 = tok->astOperand1()->getValue(0);
2347-
const ValueFlow::Value *v2 = tok->astOperand2()->getValue(0);
2348-
2349-
if (Token::Match(tok, "<|<=") && v2 && v2->isKnown()) {
2350-
const ValueType* vt = tok->astOperand1()->valueType();
2351-
if (vt && vt->pointer)
2352-
pointerLessThanZeroError(tok, v2);
2353-
if (vt && vt->sign == ValueType::UNSIGNED)
2354-
unsignedLessThanZeroError(tok, v2, tok->astOperand1()->expressionString());
2355-
} else if (Token::Match(tok, ">|>=") && v1 && v1->isKnown()) {
2356-
const ValueType* vt = tok->astOperand2()->valueType();
2357-
if (vt && vt->pointer)
2358-
pointerLessThanZeroError(tok, v1);
2359-
if (vt && vt->sign == ValueType::UNSIGNED)
2360-
unsignedLessThanZeroError(tok, v1, tok->astOperand2()->expressionString());
2361-
} else if (Token::simpleMatch(tok, ">=") && v2 && v2->isKnown()) {
2362-
const ValueType* vt = tok->astOperand1()->valueType();
2363-
if (vt && vt->pointer)
2364-
pointerPositiveError(tok, v2);
2365-
if (vt && vt->sign == ValueType::UNSIGNED)
2366-
unsignedPositiveError(tok, v2, tok->astOperand1()->expressionString());
2367-
} else if (Token::simpleMatch(tok, "<=") && v1 && v1->isKnown()) {
2368-
const ValueType* vt = tok->astOperand2()->valueType();
2369-
if (vt && vt->pointer)
2370-
pointerPositiveError(tok, v1);
2371-
if (vt && vt->sign == ValueType::UNSIGNED)
2372-
unsignedPositiveError(tok, v1, tok->astOperand2()->expressionString());
2343+
const ValueFlow::Value *zeroValue = nullptr;
2344+
const Token *nonZeroExpr = nullptr;
2345+
if (comparisonNonZeroExpressionLessThanZero(tok, &zeroValue, &nonZeroExpr)) {
2346+
const ValueType* vt = nonZeroExpr->valueType();
2347+
if (vt->pointer)
2348+
pointerLessThanZeroError(tok, zeroValue);
2349+
else
2350+
unsignedLessThanZeroError(tok, zeroValue, nonZeroExpr->expressionString());
2351+
} else if (testIfNonZeroExpressionIsPositive(tok, &zeroValue, &nonZeroExpr)) {
2352+
const ValueType* vt = nonZeroExpr->valueType();
2353+
if (vt->pointer)
2354+
pointerPositiveError(tok, zeroValue);
2355+
else
2356+
unsignedPositiveError(tok, zeroValue, nonZeroExpr->expressionString());
23732357
}
23742358
}
23752359
}
23762360
}
23772361

2362+
bool CheckOther::comparisonNonZeroExpressionLessThanZero(const Token *tok, const ValueFlow::Value **zeroValue, const Token **nonZeroExpr)
2363+
{
2364+
if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2())
2365+
return false;
2366+
2367+
const ValueFlow::Value *v1 = tok->astOperand1()->getValue(0);
2368+
const ValueFlow::Value *v2 = tok->astOperand2()->getValue(0);
2369+
2370+
if (Token::Match(tok, "<|<=") && v2 && v2->isKnown()) {
2371+
*zeroValue = v2;
2372+
*nonZeroExpr = tok->astOperand1();
2373+
} else if (Token::Match(tok, ">|>=") && v1 && v1->isKnown()) {
2374+
*zeroValue = v1;
2375+
*nonZeroExpr = tok->astOperand2();
2376+
} else {
2377+
return false;
2378+
}
2379+
2380+
const ValueType* vt = (*nonZeroExpr)->valueType();
2381+
return vt && (vt->pointer || vt->sign == ValueType::UNSIGNED);
2382+
}
2383+
2384+
bool CheckOther::testIfNonZeroExpressionIsPositive(const Token *tok, const ValueFlow::Value **zeroValue, const Token **nonZeroExpr)
2385+
{
2386+
if (!tok->isComparisonOp() || !tok->astOperand1() || !tok->astOperand2())
2387+
return false;
2388+
2389+
const ValueFlow::Value *v1 = tok->astOperand1()->getValue(0);
2390+
const ValueFlow::Value *v2 = tok->astOperand2()->getValue(0);
2391+
2392+
if (Token::simpleMatch(tok, ">=") && v2 && v2->isKnown()) {
2393+
*zeroValue = v2;
2394+
*nonZeroExpr = tok->astOperand1();
2395+
} else if (Token::simpleMatch(tok, "<=") && v1 && v1->isKnown()) {
2396+
*zeroValue = v1;
2397+
*nonZeroExpr = tok->astOperand2();
2398+
} else {
2399+
return false;
2400+
}
2401+
2402+
const ValueType* vt = (*nonZeroExpr)->valueType();
2403+
return vt && (vt->pointer || vt->sign == ValueType::UNSIGNED);
2404+
}
2405+
23782406
void CheckOther::unsignedLessThanZeroError(const Token *tok, const ValueFlow::Value * v, const std::string &varname)
23792407
{
23802408
reportError(getErrorPath(tok, v, "Unsigned less than zero"), Severity::style, "unsignedLessThanZero",

‎lib/checkother.h‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,13 @@ class CPPCHECKLIB CheckOther : public Check {
104104
checkOther.checkModuloOfOne();
105105
}
106106

107+
/** Is expression a comparison that checks if a nonzero (unsigned/pointer) expression is less than zero? */
108+
static bool comparisonNonZeroExpressionLessThanZero(const Token *tok, const ValueFlow::Value **zeroValue, const Token **nonZeroExpr);
109+
110+
/** Is expression a comparison that checks if a nonzero (unsigned/pointer) expression is positive? */
111+
static bool testIfNonZeroExpressionIsPositive(const Token *tok, const ValueFlow::Value **zeroValue, const Token **nonZeroExpr);
112+
113+
107114
/** @brief Clarify calculation for ".. a * b ? .." */
108115
void clarifyCalculation();
109116

‎test/testcondition.cpp‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2375,8 +2375,8 @@ class TestCondition : public TestFixture {
23752375
check("void f1(const std::string &s) { if(s.size() > 0) if(s.empty()) {}}");
23762376
ASSERT_EQUALS("[test.cpp:1] -> [test.cpp:1]: (warning) Opposite inner 'if' condition leads to a dead code block.\n", errout.str());
23772377

2378-
check("void f1(const std::string &s) { if(s.size() < 0) if(s.empty()) {}} ");
2379-
ASSERT_EQUALS("[test.cpp:1]: (style) Condition 's.size()<0' is always false\n", errout.str());
2378+
check("void f1(const std::string &s) { if(s.size() < 0) if(s.empty()) {}} "); // <- CheckOther reports: checking if unsigned expression is less than zero
2379+
ASSERT_EQUALS("", errout.str());
23802380

23812381
check("void f1(const std::string &s) { if(s.empty()) if(s.size() > 42) {}}");
23822382
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 @@ class TestCondition : public TestFixture {
24112411
check("void f1(const std::string &s) { if(s.size() < 2) if(s.empty()) {}}");
24122412
ASSERT_EQUALS("", errout.str());
24132413

2414-
check("void f1(const std::string &s) { if(s.size() >= 0) if(s.empty()) {}} ");
2415-
ASSERT_EQUALS("[test.cpp:1]: (style) Condition 's.size()>=0' is always true\n", errout.str());
2414+
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]
2415+
ASSERT_EQUALS("", errout.str());
24162416

24172417
// TODO: These are identical condition since size cannot be negative
24182418
check("void f1(const std::string &s) { if(s.size() <= 0) if(s.empty()) {}}");
@@ -3632,6 +3632,13 @@ class TestCondition : public TestFixture {
36323632
" if (x == -1) {}\n"
36333633
"}\n");
36343634
ASSERT_EQUALS("", errout.str());
3635+
3636+
// do not report both unsignedLessThanZero and knownConditionTrueFalse
3637+
check("void foo(unsigned int max) {\n"
3638+
" unsigned int num = max - 1;\n"
3639+
" if (num < 0) {}\n" // <- do not report knownConditionTrueFalse
3640+
"}");
3641+
ASSERT_EQUALS("", errout.str());
36353642
}
36363643

36373644
void alwaysTrueInfer() {

0 commit comments

Comments
 (0)