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

Refactor knownConditionTrueFalse check and isUsedAsBool function (#4432) 

* Refactor knownConditionTrueFalse check and isUsedAsBool function

* Format1

* Format

* Skip assign
This commit is contained in:
Paul Fultz II 2022-09-04 03:24:45 -05:00 committed by GitHub
parent f93b588603
commit 6ce5c24f21
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 163 additions and 177 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

161
lib/astutils.cpp
View File

@ -633,6 +633,89 @@ const Token* getParentLifetime(bool cpp, const Token* tok, const Library* librar
return *it;
}
static bool isInConstructorList(const Token* tok)
{
if (!tok)
return false;
if (!astIsRHS(tok))
return false;
const Token* parent = tok->astParent();
if (!Token::Match(parent, "{|("))
return false;
if (!Token::Match(parent->previous(), "%var% {|("))
return false;
if (!parent->astOperand1() || !parent->astOperand2())
return false;
do {
parent = parent->astParent();
} while (Token::simpleMatch(parent, ","));
return Token::simpleMatch(parent, ":") && !Token::simpleMatch(parent->astParent(), "?");
}
std::vector<ValueType> getParentValueTypes(const Token* tok, const Settings* settings, const Token** parent)
{
if (!tok)
return {};
if (!tok->astParent())
return {};
if (isInConstructorList(tok)) {
if (parent)
*parent = tok->astParent()->astOperand1();
if (tok->astParent()->astOperand1()->valueType())
return {*tok->astParent()->astOperand1()->valueType()};
return {};
} else if (Token::Match(tok->astParent(), "(|{|,")) {
int argn = -1;
const Token* ftok = getTokenArgumentFunction(tok, argn);
const Token* typeTok = nullptr;
if (ftok && argn >= 0) {
if (ftok->function()) {
std::vector<ValueType> result;
const Token* nameTok = nullptr;
for (const Variable* var : getArgumentVars(ftok, argn)) {
if (!var)
continue;
if (!var->valueType())
continue;
nameTok = var->nameToken();
result.push_back(*var->valueType());
}
if (result.size() == 1 && nameTok && parent) {
*parent = nameTok;
}
return result;
} else if (const Type* t = Token::typeOf(ftok, &typeTok)) {
if (astIsPointer(typeTok))
return {*typeTok->valueType()};
const Scope* scope = t->classScope;
// Check for aggregate constructors
if (scope && scope->numConstructors == 0 && t->derivedFrom.empty() &&
(t->isClassType() || t->isStructType()) && numberOfArguments(ftok) < scope->varlist.size()) {
assert(argn < scope->varlist.size());
auto it = std::next(scope->varlist.begin(), argn);
if (it->valueType())
return {*it->valueType()};
}
}
}
}
if (settings && Token::Match(tok->astParent()->tokAt(-2), ". push_back|push_front|insert|push (") &&
astIsContainer(tok->astParent()->tokAt(-2)->astOperand1())) {
const Token* contTok = tok->astParent()->tokAt(-2)->astOperand1();
const ValueType* vtCont = contTok->valueType();
if (!vtCont->containerTypeToken)
return {};
ValueType vtParent = ValueType::parseDecl(vtCont->containerTypeToken, settings, true); // TODO: set isCpp
return {std::move(vtParent)};
}
if (Token::Match(tok->astParent(), "return|(|{|%assign%") && parent) {
*parent = tok->astParent();
}
if (tok->astParent()->valueType())
return {*tok->astParent()->valueType()};
return {};
}
bool astIsLHS(const Token* tok)
{
if (!tok)
@ -1261,69 +1344,39 @@ static bool isForLoopCondition(const Token * const tok)
parent->astParent()->astParent()->astOperand1()->str() == "for";
}
static bool isZeroConstant(const Token *tok)
{
while (tok && tok->isCast())
tok = tok->astOperand2() ? tok->astOperand2() : tok->astOperand1();
return Token::simpleMatch(tok, "0") && !tok->isExpandedMacro();
}
/**
* Is token used a boolean (cast to a bool, or used as a condition somewhere)
* @param tok the token to check
* @param checkingParent true if we are checking a parent. This is used to know
* what we are checking. For instance in `if (i == 2)`, isUsedAsBool("==") is
* true whereas isUsedAsBool("i") is false, but it might call
* isUsedAsBool_internal("==") which must not return true
*/
static bool isUsedAsBool_internal(const Token * const tok, bool checkingParent)
bool isUsedAsBool(const Token* const tok)
{
if (!tok)
return false;
const Token::Type type = tok->tokType();
if (type == Token::eBitOp || type == Token::eIncDecOp || (type == Token::eArithmeticalOp && !tok->isUnaryOp("*")))
// those operators don't return a bool
return false;
if (type == Token::eComparisonOp) {
if (!checkingParent)
// this operator returns a bool
return true;
if (Token::Match(tok, "==|!="))
return isZeroConstant(tok->astOperand1()) || isZeroConstant(tok->astOperand2());
return false;
}
if (type == Token::eLogicalOp)
return true;
if (astIsBool(tok))
return true;
const Token * const parent = tok->astParent();
if (Token::Match(tok, "!|&&|%oror%|%comp%"))
return true;
const Token* parent = tok->astParent();
if (!parent)
return false;
if (parent->str() == "(" && parent->astOperand2() == tok) {
if (Token::Match(parent->astOperand1(), "if|while"))
return true;
if (!parent->isCast()) { // casts are handled via the recursive call, as astIsBool will be true
// is it a call to a function ?
int argnr;
const Token *const func = getTokenArgumentFunction(tok, argnr);
if (!func || !func->function())
return false;
const Variable *var = func->function()->getArgumentVar(argnr);
return var && (var->getTypeName() == "bool");
}
} else if (isForLoopCondition(tok))
if (Token::Match(parent, "&&|!|%oror%"))
return true;
else if (Token::simpleMatch(parent, "?") && astIsLHS(tok))
if (parent->isCast())
return isUsedAsBool(parent);
if (parent->isUnaryOp("*"))
return isUsedAsBool(parent);
if (Token::Match(parent, "==|!=") && tok->astSibling()->hasKnownIntValue() &&
tok->astSibling()->values().front().intvalue == 0)
return true;
return isUsedAsBool_internal(parent, true);
}
bool isUsedAsBool(const Token * const tok)
{
return isUsedAsBool_internal(tok, false);
if (parent->str() == "(" && astIsRHS(tok) && Token::Match(parent->astOperand1(), "if|while"))
return true;
if (Token::simpleMatch(parent, "?") && astIsLHS(tok))
return true;
if (isForLoopCondition(tok))
return true;
if (!Token::Match(parent, "%cop%")) {
std::vector<ValueType> vtParents = getParentValueTypes(tok);
return std::any_of(vtParents.begin(), vtParents.end(), [&](const ValueType& vt) {
return vt.pointer == 0 && vt.type == ValueType::BOOL;
});
}
return false;
}
static bool astIsBoolLike(const Token* tok)

4
lib/astutils.h
View File

@ -180,6 +180,10 @@ const Token* getParentMember(const Token * tok);
const Token* getParentLifetime(const Token* tok);
const Token* getParentLifetime(bool cpp, const Token* tok, const Library* library);
std::vector<ValueType> getParentValueTypes(const Token* tok,
const Settings* settings = nullptr,
const Token** parent = nullptr);
bool astIsLHS(const Token* tok);
bool astIsRHS(const Token* tok);

61
lib/checkcondition.cpp
View File

@ -1467,6 +1467,7 @@ void CheckCondition::alwaysTrueFalse()
if (f->functionScope && Token::Match(f->functionScope->bodyStart, "{ return true|false ;"))
continue;
}
const Token* condition = nullptr;
{
// is this a condition..
const Token *parent = tok->astParent();
@ -1474,29 +1475,43 @@ void CheckCondition::alwaysTrueFalse()
parent = parent->astParent();
if (!parent)
continue;
const Token *condition = nullptr;
if (parent->str() == "?" && precedes(tok, parent))
condition = parent->astOperand1();
condition = parent;
else if (Token::Match(parent->previous(), "if|while ("))
condition = parent->astOperand2();
condition = parent->previous();
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 ("))
condition = parent->astOperand1();
condition = parent;
else if (parent->str() == ";" && parent->astParent() && parent->astParent()->astParent() &&
Token::simpleMatch(parent->astParent()->astParent()->previous(), "for ("))
condition = parent->astParent()->astParent()->previous();
else
continue;
(void)condition;
}
// Skip already diagnosed values
if (diag(tok, false))
continue;
if (condition->isConstexpr())
continue;
if (!isUsedAsBool(tok))
continue;
if (Token::simpleMatch(tok->astParent(), "return") && Token::Match(tok, ".|%var%|%assign%"))
continue;
if (Token::Match(tok, "%num%|%bool%|%char%"))
continue;
if (Token::Match(tok, "! %num%|%bool%|%char%"))
continue;
if (Token::Match(tok, "%oror%|&&|:"))
if (Token::Match(tok, "%oror%|&&") &&
(tok->astOperand1()->hasKnownIntValue() || tok->astOperand2()->hasKnownIntValue()))
continue;
if (tok->isComparisonOp() && isSameExpression(mTokenizer->isCPP(), true, tok->astOperand1(), tok->astOperand2(), mSettings->library, true, true))
if (Token::simpleMatch(tok, ":"))
continue;
if (tok->isComparisonOp() && isSameExpression(mTokenizer->isCPP(),
true,
tok->astOperand1(),
tok->astOperand2(),
mSettings->library,
true,
true))
continue;
if (isConstVarExpression(tok, [](const Token* tok) {
return Token::Match(tok, "[|(|&|+|-|*|/|%|^|>>|<<") && !Token::simpleMatch(tok, "( )");
@ -1508,32 +1523,11 @@ void CheckCondition::alwaysTrueFalse()
{
const ValueFlow::Value *zeroValue = nullptr;
const Token *nonZeroExpr = nullptr;
if (CheckOther::comparisonNonZeroExpressionLessThanZero(tok, &zeroValue, &nonZeroExpr) || CheckOther::testIfNonZeroExpressionIsPositive(tok, &zeroValue, &nonZeroExpr))
if (CheckOther::comparisonNonZeroExpressionLessThanZero(tok, &zeroValue, &nonZeroExpr) ||
CheckOther::testIfNonZeroExpressionIsPositive(tok, &zeroValue, &nonZeroExpr))
continue;
}
const Token* astTop = tok->astTop();
const bool constIfWhileExpression =
tok->astParent() && Token::Match(astTop->astOperand1(), "if|while") && !astTop->astOperand1()->isConstexpr() &&
(Token::Match(tok->astParent(), "%oror%|&&") || Token::Match(tok->astParent()->astOperand1(), "if|while"));
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 ternaryExpression = Token::simpleMatch(tok->astParent(), "?");
const bool isReturn = Token::simpleMatch(astTop, "return");
const bool returnExpression = isReturn && (tok->isComparisonOp() || Token::Match(tok, "&&|%oror%"));
const bool callExpression = Token::simpleMatch(tok, "(");
if (!(constIfWhileExpression || constValExpr || compExpr || ternaryExpression || returnExpression || callExpression))
continue;
// only warn for functions returning bool
if (isReturn && callExpression && !(scope->function && scope->function->retDef && scope->function->retDef->str() == "bool"))
continue;
const Token* expr1 = tok->astOperand1(), *expr2 = tok->astOperand2();
const bool isUnknown = (expr1 && expr1->valueType() && expr1->valueType()->type == ValueType::UNKNOWN_TYPE) ||
(expr2 && expr2->valueType() && expr2->valueType()->type == ValueType::UNKNOWN_TYPE);
if (isUnknown)
continue;
// Don't warn when there are expanded macros..
bool isExpandedMacro = false;
visitAstNodes(tok, [&](const Token * tok2) {
@ -1575,9 +1569,6 @@ void CheckCondition::alwaysTrueFalse()
if (hasSizeof)
continue;
if (isIfConstexpr(tok))
continue;
alwaysTrueFalseError(tok, &tok->values().front());
}
}

109
lib/valueflow.cpp
View File

@ -378,91 +378,6 @@ const Token *parseCompareInt(const Token *tok, ValueFlow::Value &true_value, Val
});
}
static bool isInConstructorList(const Token* tok)
{
if (!tok)
return false;
if (!astIsRHS(tok))
return false;
const Token* parent = tok->astParent();
if (!Token::Match(parent, "{|("))
return false;
if (!Token::Match(parent->previous(), "%var% {|("))
return false;
if (!parent->astOperand1() || !parent->astOperand2())
return false;
do {
parent = parent->astParent();
} while (Token::simpleMatch(parent, ","));
return Token::simpleMatch(parent, ":") && !Token::simpleMatch(parent->astParent(), "?");
}
static std::vector<ValueType> getParentValueTypes(const Token* tok,
const Settings* settings = nullptr,
const Token** parent = nullptr)
{
if (!tok)
return {};
if (!tok->astParent())
return {};
if (isInConstructorList(tok)) {
if (parent)
*parent = tok->astParent()->astOperand1();
if (tok->astParent()->astOperand1()->valueType())
return {*tok->astParent()->astOperand1()->valueType()};
return {};
} else if (Token::Match(tok->astParent(), "(|{|,")) {
int argn = -1;
const Token* ftok = getTokenArgumentFunction(tok, argn);
const Token* typeTok = nullptr;
if (ftok && argn >= 0) {
if (ftok->function()) {
std::vector<ValueType> result;
const Token* nameTok = nullptr;
for (const Variable* var : getArgumentVars(ftok, argn)) {
if (!var)
continue;
if (!var->valueType())
continue;
nameTok = var->nameToken();
result.push_back(*var->valueType());
}
if (result.size() == 1 && nameTok && parent) {
*parent = nameTok;
}
return result;
} else if (const Type* t = Token::typeOf(ftok, &typeTok)) {
if (astIsPointer(typeTok))
return {*typeTok->valueType()};
const Scope* scope = t->classScope;
// Check for aggregate constructors
if (scope && scope->numConstructors == 0 && t->derivedFrom.empty() &&
(t->isClassType() || t->isStructType()) && numberOfArguments(ftok) < scope->varlist.size()) {
assert(argn < scope->varlist.size());
auto it = std::next(scope->varlist.begin(), argn);
if (it->valueType())
return {*it->valueType()};
}
}
}
}
if (settings && Token::Match(tok->astParent()->tokAt(-2), ". push_back|push_front|insert|push (") &&
astIsContainer(tok->astParent()->tokAt(-2)->astOperand1())) {
const Token* contTok = tok->astParent()->tokAt(-2)->astOperand1();
const ValueType* vtCont = contTok->valueType();
if (!vtCont->containerTypeToken)
return {};
ValueType vtParent = ValueType::parseDecl(vtCont->containerTypeToken, settings, true); // TODO: set isCpp
return {std::move(vtParent)};
}
if (Token::Match(tok->astParent(), "return|(|{|%assign%") && parent) {
*parent = tok->astParent();
}
if (tok->astParent()->valueType())
return {*tok->astParent()->valueType()};
return {};
}
static bool isEscapeScope(const Token* tok, TokenList * tokenlist, bool unknown = false)
{
if (!Token::simpleMatch(tok, "{"))
@ -6434,6 +6349,28 @@ struct StartIteratorInferModel : IteratorInferModel {
}
};
static bool isIntegralOnlyOperator(const Token* tok) {
return Token::Match(tok, "%|<<|>>|&|^|~|%or%");
}
static bool isIntegral(const Token* tok)
{
if (!tok)
return false;
if (astIsIntegral(tok, false))
return true;
if (tok->valueType())
return false;
// These operators only work on integers
if (isIntegralOnlyOperator(tok))
return true;
if (isIntegralOnlyOperator(tok->astParent()))
return true;
if (Token::Match(tok, "+|-|*|/") && tok->isBinaryOp())
return isIntegral(tok->astOperand1()) && isIntegral(tok->astOperand2());
return false;
}
static void valueFlowInferCondition(TokenList* tokenlist,
const Settings* settings)
{
@ -6463,7 +6400,7 @@ static void valueFlowInferCondition(TokenList* tokenlist,
setTokenValue(tok, value, settings);
}
}
} else {
} else if (isIntegral(tok->astOperand1()) && isIntegral(tok->astOperand2())) {
std::vector<ValueFlow::Value> result =
infer(IntegralInferModel{}, tok->str(), tok->astOperand1()->values(), tok->astOperand2()->values());
for (const ValueFlow::Value& value : result) {

5
test/testcondition.cpp
View File

@ -3336,7 +3336,7 @@ private:
" const int b = 52;\n"
" return a+b;\n"
"}");
ASSERT_EQUALS("", errout.str());
ASSERT_EQUALS("[test.cpp:4]: (style) Condition 'a+b' is always true\n", errout.str());
check("int f() {\n"
" int a = 50;\n"
@ -4318,7 +4318,8 @@ private:
ASSERT_EQUALS("[test.cpp:3]: (style) Condition '!s.empty()' is always false\n"
"[test.cpp:4]: (style) Condition 's.empty()' is always true\n"
"[test.cpp:5]: (style) Condition 's.empty()' is always true\n"
"[test.cpp:6]: (style) Condition '(bool)0' is always false\n",
"[test.cpp:6]: (style) Condition '(bool)0' is always false\n"
"[test.cpp:7]: (style) Condition 's.empty()' is always true\n",
errout.str());
check("int f(bool b) {\n"