From 08184f4681815df2fffa415991d92f3b15c86f26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Tue, 4 May 2021 13:46:46 +0200 Subject: [PATCH] Fixed #10070 (FP warning: Logical disjunction always evaluates to true) --- lib/astutils.cpp | 143 +++++++++++++++++++++-------------------- test/testastutils.cpp | 4 +- test/testcondition.cpp | 8 +++ 3 files changed, 85 insertions(+), 70 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 7d4f5d1fe..504b780d7 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -931,9 +931,83 @@ static bool isSameConstantValue(bool macro, const Token * const tok1, const Toke return isEqualKnownValue(tok1, tok2); } + +static bool isForLoopCondition(const Token * const tok) +{ + if (!tok) + return false; + const Token *const parent = tok->astParent(); + return Token::simpleMatch(parent, ";") && parent->astOperand1() == tok && + Token::simpleMatch(parent->astParent(), ";") && + Token::simpleMatch(parent->astParent()->astParent(), "(") && + 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) +{ + 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 (!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)) + return true; + + return isUsedAsBool_internal(parent, true); +} + +bool isUsedAsBool(const Token * const tok) +{ + return isUsedAsBool_internal(tok, false); +} + static bool astIsBoolLike(const Token* tok) { - return astIsBool(tok) || astIsPointer(tok) || astIsSmartPointer(tok); + return astIsBool(tok) || isUsedAsBool(tok); } bool isSameExpression(bool cpp, bool macro, const Token *tok1, const Token *tok2, const Library& library, bool pure, bool followVar, ErrorPath* errors) @@ -1158,73 +1232,6 @@ static bool isZeroBoundCond(const Token * const cond) return false; } -static bool isForLoopCondition(const Token * const tok) -{ - if (!tok) - return false; - const Token *const parent = tok->astParent(); - return Token::simpleMatch(parent, ";") && parent->astOperand1() == tok && - Token::simpleMatch(parent->astParent(), ";") && - Token::simpleMatch(parent->astParent()->astParent(), "(") && - parent->astParent()->astParent()->astOperand1()->str() == "for"; -} - -/** - * 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) -{ - 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 astIsBool(tok->astOperand1()) || astIsBool(tok->astOperand2()); - return false; - } - if (type == Token::eLogicalOp) - return true; - if (astIsBool(tok)) - return true; - - const Token * const 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)) - return true; - - return isUsedAsBool_internal(parent, true); -} - -bool isUsedAsBool(const Token * const tok) -{ - return isUsedAsBool_internal(tok, false); -} - bool isOppositeCond(bool isNot, bool cpp, const Token * const cond1, const Token * const cond2, const Library& library, bool pure, bool followVar, ErrorPath* errors) { if (!cond1 || !cond2) diff --git a/test/testastutils.cpp b/test/testastutils.cpp index 0bbeda54b..41831050f 100644 --- a/test/testastutils.cpp +++ b/test/testastutils.cpp @@ -291,8 +291,8 @@ private: ASSERT(Result::False == isUsedAsBool("void f() { int i; for (i;;) {} }", "i ; ; )")); ASSERT(Result::True == isUsedAsBool("void f() { int i; for (int j=0; i; ++j) {} }", "i ; ++")); ASSERT(Result::False == isUsedAsBool("void f() { int i; if (i == 2) {} }", "i ==")); - ASSERT(Result::True == isUsedAsBool("void f() { int i; if (i == true) {} }", "i ==")); - ASSERT(Result::True == isUsedAsBool("void f() { int i,j; if (i == (j&&f())) {} }", "i ==")); + ASSERT(Result::False == isUsedAsBool("void f() { int i; if (i == true) {} }", "i ==")); + ASSERT(Result::False == isUsedAsBool("void f() { int i,j; if (i == (j&&f())) {} }", "i ==")); ASSERT(Result::True == isUsedAsBool("void f() { int i; if (!i == 0) {} }", "i ==")); ASSERT(Result::True == isUsedAsBool("void f() { int i; if (!i) {} }", "i )")); ASSERT(Result::True == isUsedAsBool("void f() { int i; if (!!i) {} }", "i )")); diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 13d2a1c13..c431fcf91 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -78,6 +78,7 @@ private: TEST_CASE(incorrectLogicOperator13); TEST_CASE(incorrectLogicOperator14); TEST_CASE(incorrectLogicOperator15); + TEST_CASE(incorrectLogicOperator16); // #10070 TEST_CASE(secondAlwaysTrueFalseWhenFirstTrueError); TEST_CASE(incorrectLogicOp_condSwapping); TEST_CASE(testBug5895); @@ -1579,6 +1580,13 @@ private: ASSERT_EQUALS("", errout.str()); } + void incorrectLogicOperator16() { // #10070 + check("void foo(void* p) {\n" + " if (!p || p == -1) { }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void secondAlwaysTrueFalseWhenFirstTrueError() { check("void f(int x) {\n" " if (x > 5 && x != 1)\n"