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

Fix issue 9578: false negative: (style) Condition '...' is always false

This commit is contained in:
Paul 2020-06-28 15:28:08 -05:00
parent 0e736e0c29
commit d5b6d49d96
5 changed files with 72 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
lib/astutils.cpp
View File

@ -20,6 +20,7 @@
//--------------------------------------------------------------------------- //---------------------------------------------------------------------------
#include "astutils.h" #include "astutils.h"
#include "config.h"
#include "library.h" #include "library.h"
#include "mathlib.h" #include "mathlib.h"
#include "settings.h" #include "settings.h"
@ -32,13 +33,13 @@
#include <list> #include <list>
#include <stack> #include <stack>
template<class T, REQUIRES("T must be a Token class", std::is_convertible<T*, const Token*>)>
void visitAstNodes(const Token *ast, std::function<ChildrenToVisit(const Token *)> visitor) void visitAstNodesGeneric(T *ast, std::function<ChildrenToVisit(T *)> visitor)
{ {
std::stack<const Token *> tokens; std::stack<T *> tokens;
tokens.push(ast); tokens.push(ast);
while (!tokens.empty()) { while (!tokens.empty()) {
const Token *tok = tokens.top(); T *tok = tokens.top();
tokens.pop(); tokens.pop();
if (!tok) if (!tok)
continue; continue;
@ -54,6 +55,16 @@ void visitAstNodes(const Token *ast, std::function<ChildrenToVisit(const Token *
} }
} }
void visitAstNodes(const Token *ast, std::function<ChildrenToVisit(const Token *)> visitor)
{
visitAstNodesGeneric(ast, std::move(visitor));
}
void visitAstNodes(Token *ast, std::function<ChildrenToVisit(Token *)> visitor)
{
visitAstNodesGeneric(ast, std::move(visitor));
}
static void astFlattenRecursive(const Token *tok, std::vector<const Token *> *result, const char* op, nonneg int depth = 0) static void astFlattenRecursive(const Token *tok, std::vector<const Token *> *result, const char* op, nonneg int depth = 0)
{ {
++depth; ++depth;

1
lib/astutils.h
View File

@ -47,6 +47,7 @@ enum class ChildrenToVisit {
* Visit AST nodes recursively. The order is not "well defined" * Visit AST nodes recursively. The order is not "well defined"
*/ */
void visitAstNodes(const Token *ast, std::function<ChildrenToVisit(const Token *)> visitor); void visitAstNodes(const Token *ast, std::function<ChildrenToVisit(const Token *)> visitor);
void visitAstNodes(Token *ast, std::function<ChildrenToVisit(Token *)> visitor);
std::vector<const Token*> astFlatten(const Token* tok, const char* op); std::vector<const Token*> astFlatten(const Token* tok, const char* op);

5
lib/checkcondition.cpp
View File

@ -1401,6 +1401,8 @@ void CheckCondition::alwaysTrueFalse()
condition = parent->astOperand1(); condition = parent->astOperand1();
else if (Token::Match(parent->previous(), "if|while (")) else if (Token::Match(parent->previous(), "if|while ("))
condition = parent->astOperand2(); condition = parent->astOperand2();
else if (Token::simpleMatch(parent, "return"))
condition = parent->astOperand1();
else if (parent->str() == ";" && parent->astParent() && parent->astParent()->astParent() && Token::simpleMatch(parent->astParent()->astParent()->previous(), "for (")) else if (parent->str() == ";" && parent->astParent() && parent->astParent()->astParent() && Token::simpleMatch(parent->astParent()->astParent()->previous(), "for ("))
condition = parent->astOperand1(); condition = parent->astOperand1();
else else
@ -1427,8 +1429,9 @@ void CheckCondition::alwaysTrueFalse()
const bool constValExpr = tok->isNumber() && Token::Match(tok->astParent(),"%oror%|&&|?"); // just one number in boolean expression const bool constValExpr = tok->isNumber() && Token::Match(tok->astParent(),"%oror%|&&|?"); // just one number in boolean expression
const bool compExpr = Token::Match(tok, "%comp%|!"); // a compare expression const bool compExpr = Token::Match(tok, "%comp%|!"); // a compare expression
const bool ternaryExpression = Token::simpleMatch(tok->astParent(), "?"); const bool ternaryExpression = Token::simpleMatch(tok->astParent(), "?");
const bool returnExpression = Token::simpleMatch(tok->astTop(), "return") && (tok->isComparisonOp() || Token::Match(tok, "&&|%oror%"));
if (!(constIfWhileExpression || constValExpr || compExpr || ternaryExpression)) if (!(constIfWhileExpression || constValExpr || compExpr || ternaryExpression || returnExpression))
continue; continue;
// Don't warn when there are expanded macros.. // Don't warn when there are expanded macros..

45
lib/valueflow.cpp
View File

@ -4071,39 +4071,36 @@ struct ValueFlowConditionHandler {
if (Token::Match(tok->astParent(), "%oror%|&&")) { if (Token::Match(tok->astParent(), "%oror%|&&")) {
Token *parent = tok->astParent(); Token *parent = tok->astParent();
const std::string &op(parent->str()); if (astIsRHS(tok) && parent->astParent() && parent->str() == parent->astParent()->str())
parent = parent->astParent();
if (parent->astOperand1() == tok && ((op == "&&" && Token::Match(tok, "==|>=|<=|!")) || else if (!astIsLHS(tok)) {
(op == "||" && Token::Match(tok, "%name%|!=")))) { parent = nullptr;
for (; parent && parent->str() == op; parent = parent->astParent()) { }
std::stack<Token *> tokens; if (parent) {
tokens.push(parent->astOperand2()); const std::string &op(parent->str());
bool assign = false; bool assign = false;
while (!tokens.empty()) { std::list<ValueFlow::Value> values = cond.true_values;
Token *rhstok = tokens.top(); if (Token::Match(tok, "==|!="))
tokens.pop(); changePossibleToKnown(values);
if (!rhstok) if ((op == "&&" && Token::Match(tok, "==|>=|<=|!")) ||
continue; (op == "||" && Token::Match(tok, "%name%|!="))) {
tokens.push(rhstok->astOperand1()); visitAstNodes(parent->astOperand2(), [&](Token* tok2) {
tokens.push(rhstok->astOperand2()); if (isSameExpression(tokenlist->isCPP(), false, cond.vartok, tok2, settings->library, true, false))
if (isSameExpression( setTokenValue(tok2, values.front(), settings);
tokenlist->isCPP(), false, cond.vartok, rhstok, settings->library, true, false)) else if (Token::Match(tok2, "++|--|=") && isSameExpression(tokenlist->isCPP(),
setTokenValue(rhstok, cond.true_values.front(), settings);
else if (Token::Match(rhstok, "++|--|=") && isSameExpression(tokenlist->isCPP(),
false, false,
cond.vartok, cond.vartok,
rhstok->astOperand1(), tok2->astOperand1(),
settings->library, settings->library,
true, true,
false)) { false)) {
assign = true; assign = true;
break; return ChildrenToVisit::done;
} }
} return ChildrenToVisit::op1_and_op2;
});
if (assign) if (assign)
break; break;
while (parent->astParent() && parent == parent->astParent()->astOperand2())
parent = parent->astParent();
} }
} }
} }

45
test/testcondition.cpp
View File

@ -835,14 +835,16 @@ private:
" a++;\n" " a++;\n"
"}\n" "}\n"
); );
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=3' is always true\n", errout.str());
check("void f(int x) {\n" check("void f(int x) {\n"
" if (1 != x || 3 != x)\n" " if (1 != x || 3 != x)\n"
" a++;\n" " a++;\n"
"}\n" "}\n"
); );
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 1 || x != 3.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition '3!=x' is always true\n", errout.str());
check("void f(int x) {\n" check("void f(int x) {\n"
" if (x<0 && !x) {}\n" " if (x<0 && !x) {}\n"
@ -852,12 +854,14 @@ private:
check("void f(int x) {\n" check("void f(int x) {\n"
" if (x==0 && x) {}\n" " if (x==0 && x) {}\n"
"}\n"); "}\n");
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 0 && x.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 0 && x.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x' is always false\n", errout.str());
check("void f(int x) {\n" // ast.. check("void f(int x) {\n" // ast..
" if (y == 1 && x == 1 && x == 7) { }\n" " if (y == 1 && x == 1 && x == 7) { }\n"
"}\n"); "}\n");
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 7.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 7.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==7' is always false\n", errout.str());
check("void f(int x, int y) {\n" check("void f(int x, int y) {\n"
" if (x != 1 || y != 1)\n" " if (x != 1 || y != 1)\n"
@ -878,7 +882,7 @@ private:
" a++;\n" " a++;\n"
"}\n" "}\n"
); );
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=3' is always true\n", errout.str());
check("void f(int x) {\n" check("void f(int x) {\n"
" if ((x != 1) && (x != 3))\n" " if ((x != 1) && (x != 3))\n"
@ -921,7 +925,8 @@ private:
" a++;\n" " a++;\n"
"}\n" "}\n"
); );
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 5 || x != 6.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (warning) Logical disjunction always evaluates to true: x != 5 || x != 6.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=6' is always true\n", errout.str());
check("void f(unsigned int a, unsigned int b, unsigned int c) {\n" check("void f(unsigned int a, unsigned int b, unsigned int c) {\n"
" if((a != b) || (c != b) || (c != a))\n" " if((a != b) || (c != b) || (c != a))\n"
@ -945,13 +950,14 @@ private:
" if ((x == 1) && (x == 0x00000001))\n" " if ((x == 1) && (x == 0x00000001))\n"
" a++;\n" " a++;\n"
"}"); "}");
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==0x00000001' is always true\n", errout.str());
check("void f(int x) {\n" check("void f(int x) {\n"
" if (x == 1 && x == 3)\n" " if (x == 1 && x == 3)\n"
" a++;\n" " a++;\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 3.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 1 && x == 3.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==3' is always false\n", errout.str());
check("void f(int x) {\n" check("void f(int x) {\n"
" if (x == 1.0 && x == 3.0)\n" " if (x == 1.0 && x == 3.0)\n"
@ -1074,7 +1080,8 @@ private:
" a++;\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: If 'x == 3', the comparison 'x != 4' is always true.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=4' is always true\n", errout.str());
check("void f(int x) {\n" check("void f(int x) {\n"
" if ((x!=4) && (x==3))\n" " if ((x!=4) && (x==3))\n"
@ -1092,13 +1099,15 @@ private:
" if ((x!=4) || (x==3))\n" " if ((x!=4) || (x==3))\n"
" a++;\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: If 'x == 3', the comparison 'x != 4' is always true.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==3' is always false\n", errout.str());
check("void f(int x) {\n" check("void f(int x) {\n"
" if ((x==3) && (x!=3))\n" " if ((x==3) && (x!=3))\n"
" a++;\n" " a++;\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 3 && x != 3.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == 3 && x != 3.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x!=3' is always false\n", errout.str());
check("void f(int x) {\n" check("void f(int x) {\n"
" if ((x==6) || (x!=6))\n" " if ((x==6) || (x!=6))\n"
@ -1190,7 +1199,8 @@ private:
check("void f(char x) {\n" check("void f(char x) {\n"
" if (x == '1' && x == '2') {}\n" " if (x == '1' && x == '2') {}\n"
"}", "test.cpp", true); "}", "test.cpp", true);
ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == '1' && x == '2'.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x == '1' && x == '2'.\n"
"[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x=='2'' is always false\n", errout.str());
check("int f(char c) {\n" check("int f(char c) {\n"
" return (c >= 'a' && c <= 'z');\n" " return (c >= 'a' && c <= 'z');\n"
@ -1264,7 +1274,8 @@ private:
" if ((t == A) && (t == B))\n" " if ((t == A) && (t == B))\n"
" {}\n" " {}\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:3]: (warning) Logical conjunction always evaluates to false: t == 0 && t == 1.\n", errout.str()); ASSERT_EQUALS("[test.cpp:3]: (warning) Logical conjunction always evaluates to false: t == 0 && t == 1.\n"
"[test.cpp:3] -> [test.cpp:3]: (style) Condition 't==B' is always false\n", errout.str());
} }
void incorrectLogicOperator11() { void incorrectLogicOperator11() {
@ -2864,7 +2875,7 @@ private:
" if(x == 0) { x++; return x == 0; } \n" " if(x == 0) { x++; return x == 0; } \n"
" return false;\n" " return false;\n"
"}"); "}");
TODO_ASSERT_EQUALS("return value is always true?", "", errout.str()); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Condition 'x==0' is always false\n", errout.str());
check("void f() {\n" // #6898 (Token::expressionString) check("void f() {\n" // #6898 (Token::expressionString)
" int x = 0;\n" " int x = 0;\n"
@ -3757,6 +3768,12 @@ private:
check("bool f();\n" check("bool f();\n"
"void foo() { bool x = true; if(x&&f()) {}}\n"); "void foo() { bool x = true; if(x&&f()) {}}\n");
ASSERT_EQUALS("[test.cpp:2]: (style) Condition 'x' is always true\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (style) Condition 'x' is always true\n", errout.str());
// #9578
check("bool f(const std::string &s) {\n"
" return s.size()>2U && s[0]=='4' && s[0]=='2';\n"
"}\n");
ASSERT_EQUALS("[test.cpp:2]: (style) Condition 'x' is always true\n", errout.str());
} }
void pointerAdditionResultNotNull() { void pointerAdditionResultNotNull() {