walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Fix #10320 Wrong redundant condition: misleading/wrong message (#4334) 

* Fix #10320 Wrong redundant condition: misleading/wrong message

* Use expressionString()

* Clarify condition

* Update testcondition.cpp

* Trigger CI
This commit is contained in:
chrchr-github 2022-08-02 18:31:02 +02:00 committed by GitHub
parent 92d569afb6
commit bc409776e3
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 95 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

81
lib/checkcondition.cpp
View File

@ -893,6 +893,61 @@ static std::string invertOperatorForOperandSwap(std::string s)
return s;
}
template<typename T>
static int sign(const T v) {
return static_cast<int>(v > 0) - static_cast<int>(v < 0);
}
// returns 1 (-1) if the first (second) condition is sufficient, 0 if indeterminate
template<typename T>
static int sufficientCondition(std::string op1, const bool not1, const T value1, std::string op2, const bool not2, const T value2, const bool isAnd) {
auto transformOp = [](std::string& op, const bool invert) {
if (invert) {
if (op == "==")
op = "!=";
else if (op == "!=")
op = "==";
else if (op == "<")
op = ">=";
else if (op == ">")
op = "<=";
else if (op == "<=")
op = ">";
else if (op == ">=")
op = "<";
}
};
transformOp(op1, not1);
transformOp(op2, not2);
int res = 0;
bool equal = false;
if (op1 == op2) {
equal = true;
if (op1 == ">" || op1 == ">=")
res = sign(value1 - value2);
else if (op1 == "<" || op1 == "<=")
res = -sign(value1 - value2);
} else { // not equal
if (op1 == "!=")
res = 1;
else if (op2 == "!=")
res = -1;
else if (op1 == "==")
res = -1;
else if (op2 == "==")
res = 1;
else if (op1 == ">" && op2 == ">=")
res = sign(value1 - (value2 - 1));
else if (op1 == ">=" && op2 == ">")
res = sign((value1 - 1) - value2);
else if (op1 == "<" && op2 == "<=")
res = -sign(value1 - (value2 + 1));
else if (op1 == "<=" && op2 == "<")
res = -sign((value1 + 1) - value2);
}
return res * (isAnd == equal ? 1 : -1);
}
template<typename T>
static bool checkIntRelation(const std::string &op, const T value1, const T value2)
{
@ -1006,16 +1061,14 @@ static bool parseComparison(const Token *comp, bool *not1, std::string *op, std:
static std::string conditionString(bool not1, const Token *expr1, const std::string &op, const std::string &value1)
{
if (expr1->astParent()->isComparisonOp())
return std::string(not1 ? "!(" : "") +
(expr1->isName() ? expr1->str() : std::string("EXPR")) +
return std::string(not1 ? "!(" : "") + expr1->expressionString() +
" " +
op +
" " +
value1 +
(not1 ? ")" : "");
return std::string(not1 ? "!" : "") +
(expr1->isName() ? expr1->str() : std::string("EXPR"));
return std::string(not1 ? "!" : "") + expr1->expressionString();
}
static std::string conditionString(const Token * tok)
@ -1198,6 +1251,7 @@ void CheckCondition::checkIncorrectLogicOperator()
// evaluate if expression is always true/false
bool alwaysTrue = true, alwaysFalse = true;
bool firstTrue = true, secondTrue = true;
const bool isAnd = tok->str() == "&&";
for (int test = 1; test <= 5; ++test) {
// test:
// 1 => testvalue is less than both value1 and value2
@ -1223,7 +1277,7 @@ void CheckCondition::checkIncorrectLogicOperator()
result1 = !result1;
if (not2)
result2 = !result2;
if (tok->str() == "&&") {
if (isAnd) {
alwaysTrue &= (result1 && result2);
alwaysFalse &= !(result1 && result2);
} else {
@ -1239,16 +1293,13 @@ void CheckCondition::checkIncorrectLogicOperator()
if (printWarning && (alwaysTrue || alwaysFalse)) {
const std::string text = cond1str + " " + tok->str() + " " + cond2str;
incorrectLogicOperatorError(tok, text, alwaysTrue, inconclusive, errorPath);
} else if (printStyle && secondTrue) {
const std::string text = "If '" + cond1str + "', the comparison '" + cond2str +
"' is always true.";
redundantConditionError(tok, text, inconclusive);
} else if (printStyle && firstTrue) {
//const std::string text = "The comparison " + cond1str + " is always " +
// (firstTrue ? "true" : "false") + " when " +
// cond2str + ".";
const std::string text = "If '" + cond2str + "', the comparison '" + cond1str +
"' is always true.";
} else if (printStyle && (firstTrue || secondTrue)) {
const int which = sufficientCondition(op1, not1, i1, op2, not2, i2, isAnd);
std::string text;
if (which != 0) {
text = "The condition '" + (which == 1 ? cond2str : cond1str) + "' is redundant since '" + (which == 1 ? cond1str : cond2str) + "' is sufficient.";
} else
text = "If '" + (secondTrue ? cond1str : cond2str) + "', the comparison '" + (secondTrue ? cond2str : cond1str) + "' is always true.";
redundantConditionError(tok, text, inconclusive);
}
}

49
test/testcondition.cpp
View File

@ -1162,7 +1162,7 @@ private:
" if((x==3) && (x!=4))\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x != 4' is redundant since 'x == 3' is sufficient.\n", errout.str());
check("void f(const std::string &s) {\n" // #8860
" const std::size_t p = s.find(\"42\");\n"
@ -1175,19 +1175,19 @@ private:
" if ((x!=4) && (x==3))\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x != 4' is redundant since 'x == 3' is sufficient.\n", errout.str());
check("void f(int x) {\n"
" if ((x==3) || (x!=4))\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x == 3' is redundant since 'x != 4' is sufficient.\n", errout.str());
check("void f(int x) {\n"
" if ((x!=4) || (x==3))\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 3', the comparison 'x != 4' is always true.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x == 3' is redundant since 'x != 4' is sufficient.\n", errout.str());
check("void f(int x) {\n"
" if ((x==3) && (x!=3))\n"
@ -1217,7 +1217,7 @@ private:
" if (x > 5 && x == 6)\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 6', the comparison 'x > 5' is always true.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x > 5' is redundant since 'x == 6' is sufficient.\n", errout.str());
// #3419
check("void f() {\n"
@ -1281,7 +1281,7 @@ private:
check("void f(int x) {\n"
" if (x+3 > 2 || x+3 < 10) {}\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: EXPR > 2 || EXPR < 10.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x+3 > 2 || x+3 < 10.\n", errout.str());
}
void incorrectLogicOperator6() { // char literals
@ -1691,7 +1691,7 @@ private:
" if (x > 5 && x != 1)\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x > 5', the comparison 'x != 1' is always true.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x != 1' is redundant since 'x > 5' is sufficient.\n", errout.str());
check("void f(int x) {\n"
" if (x > 5 && x != 6)\n"
@ -1703,7 +1703,7 @@ private:
" if ((x > 5) && (x != 1))\n"
" a++;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x > 5', the comparison 'x != 1' is always true.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x != 1' is redundant since 'x > 5' is sufficient.\n", errout.str());
check("void f(int x) {\n"
" if ((x > 5) && (x != 6))\n"
@ -1717,10 +1717,11 @@ private:
" d = x >= 3 || x == 4;\n"
" e = x <= 5 || x == 4;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x == 4', the comparison 'x > 3' is always true.\n"
"[test.cpp:3]: (style) Redundant condition: If 'x == 4', the comparison 'x < 5' is always true.\n"
"[test.cpp:4]: (style) Redundant condition: If 'x == 4', the comparison 'x >= 3' is always true.\n"
"[test.cpp:5]: (style) Redundant condition: If 'x == 4', the comparison 'x <= 5' is always true.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x == 4' is redundant since 'x > 3' is sufficient.\n"
"[test.cpp:3]: (style) Redundant condition: The condition 'x == 4' is redundant since 'x < 5' is sufficient.\n"
"[test.cpp:4]: (style) Redundant condition: The condition 'x == 4' is redundant since 'x >= 3' is sufficient.\n"
"[test.cpp:5]: (style) Redundant condition: The condition 'x == 4' is redundant since 'x <= 5' is sufficient.\n",
errout.str());
check("void f(int x, bool& b) {\n"
" b = x > 5 || x != 1;\n"
@ -1728,10 +1729,11 @@ private:
" d = x >= 5 || x != 1;\n"
" e = x <= 1 || x != 3;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x > 5', the comparison 'x != 1' is always true.\n"
"[test.cpp:3]: (style) Redundant condition: If 'x < 1', the comparison 'x != 3' is always true.\n"
"[test.cpp:4]: (style) Redundant condition: If 'x >= 5', the comparison 'x != 1' is always true.\n"
"[test.cpp:5]: (style) Redundant condition: If 'x <= 1', the comparison 'x != 3' is always true.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x > 5' is redundant since 'x != 1' is sufficient.\n"
"[test.cpp:3]: (style) Redundant condition: The condition 'x < 1' is redundant since 'x != 3' is sufficient.\n"
"[test.cpp:4]: (style) Redundant condition: The condition 'x >= 5' is redundant since 'x != 1' is sufficient.\n"
"[test.cpp:5]: (style) Redundant condition: The condition 'x <= 1' is redundant since 'x != 3' is sufficient.\n",
errout.str());
check("void f(int x, bool& b) {\n"
" b = x > 6 && x > 5;\n"
@ -1739,10 +1741,17 @@ private:
" d = x < 6 && x < 5;\n"
" e = x < 5 || x < 6;\n"
"}");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If 'x > 6', the comparison 'x > 5' is always true.\n"
"[test.cpp:3]: (style) Redundant condition: If 'x > 6', the comparison 'x > 5' is always true.\n"
"[test.cpp:4]: (style) Redundant condition: If 'x < 5', the comparison 'x < 6' is always true.\n"
"[test.cpp:5]: (style) Redundant condition: If 'x < 5', the comparison 'x < 6' is always true.\n", errout.str());
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition 'x > 5' is redundant since 'x > 6' is sufficient.\n"
"[test.cpp:3]: (style) Redundant condition: The condition 'x > 6' is redundant since 'x > 5' is sufficient.\n"
"[test.cpp:4]: (style) Redundant condition: The condition 'x < 6' is redundant since 'x < 5' is sufficient.\n"
"[test.cpp:5]: (style) Redundant condition: The condition 'x < 5' is redundant since 'x < 6' is sufficient.\n",
errout.str());
check("void f(const char *p) {\n" // #10320
" if (!p || !*p || *p != 'x') {}\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: The condition '!*p' is redundant since '*p != 'x'' is sufficient.\n",
errout.str());
}
void incorrectLogicOp_condSwapping() {