diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 4776cbea38f..4c2909e251a 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1846,6 +1846,42 @@ void CheckConditionImpl::pointerAdditionResultNotNullError(const Token *tok, con reportError(tok, Severity::warning, "pointerAdditionResultNotNull", "Comparison is wrong. Result of '" + s + "' can't be 0 unless there is pointer overflow, and pointer overflow is undefined behaviour."); } +static bool checkBoolConditionalAssign(const Token* condTok, const Token* assignTok, bool& isRedundant) +{ + bool isNegation = false; + const Token* varTok = condTok; + if (condTok->isUnaryOp("!")) { + isNegation = true; + varTok = varTok->astOperand1(); + } else if (condTok->isBinaryOp()) { + varTok = condTok->astOperand1(); + if (varTok->hasKnownIntValue()) + varTok = condTok->astOperand2(); + } + + const ValueType* vt = varTok->variable() ? varTok->variable()->valueType() : nullptr; + if (!(vt && vt->type == ValueType::Type::BOOL && !vt->pointer)) + return false; + + if (!(assignTok->astOperand1() && assignTok->astOperand1()->varId() == varTok->varId())) + return false; + if (!(assignTok->astOperand2() && assignTok->astOperand2()->hasKnownIntValue())) + return false; + const MathLib::bigint val = assignTok->astOperand2()->getKnownIntValue(); + if (val < 0 || val > 1) + return false; + if (condTok->isBinaryOp()) { + if (!varTok->astSibling()->hasKnownIntValue()) + return false; + const MathLib::bigint compVal = varTok->astSibling()->getKnownIntValue(); + if (compVal < 0 || compVal > 1) + return false; + isNegation = (condTok->str() == "!=") == (compVal == 1); + } + isRedundant = (isNegation && val == 0) || (!isNegation && val == 1); + return true; +} + void CheckConditionImpl::checkDuplicateConditionalAssign() { if (!mSettings.severity.isEnabled(Severity::style) && !mSettings.isPremiumEnabled("duplicateConditionalAssign")) @@ -1862,7 +1898,7 @@ void CheckConditionImpl::checkDuplicateConditionalAssign() continue; const Token *blockTok = tok->linkAt(1)->next(); const Token *condTok = tok->next()->astOperand2(); - const bool isBoolVar = Token::Match(condTok, "!| %var%"); + bool isBoolVar = Token::Match(condTok, "!| %var%"); if (!isBoolVar && !Token::Match(condTok, "==|!=")) continue; if ((isBoolVar || condTok->str() == "!=") && Token::simpleMatch(blockTok->link(), "} else {")) @@ -1875,21 +1911,8 @@ void CheckConditionImpl::checkDuplicateConditionalAssign() if (nextAfterAstRightmostLeaf(assignTok) != blockTok->link()->previous()) continue; bool isRedundant = false; - if (isBoolVar) { - const bool isNegation = condTok->str() == "!"; - const Token* const varTok = isNegation ? condTok->next() : condTok; - const ValueType* vt = varTok->variable() ? varTok->variable()->valueType() : nullptr; - if (!(vt && vt->type == ValueType::Type::BOOL && !vt->pointer)) - continue; - - if (!(assignTok->astOperand1() && assignTok->astOperand1()->varId() == varTok->varId())) - continue; - if (!(assignTok->astOperand2() && assignTok->astOperand2()->hasKnownIntValue())) - continue; - const MathLib::bigint val = assignTok->astOperand2()->getKnownIntValue(); - if (val < 0 || val > 1) - continue; - isRedundant = (isNegation && val == 0) || (!isNegation && val == 1); + if (checkBoolConditionalAssign(condTok, assignTok, isRedundant)) { + isBoolVar = true; } else { // comparison if (!isSameExpression( true, condTok->astOperand1(), assignTok->astOperand1(), mSettings, true, true)) @@ -1898,17 +1921,17 @@ void CheckConditionImpl::checkDuplicateConditionalAssign() true, condTok->astOperand2(), assignTok->astOperand2(), mSettings, true, true)) continue; } - duplicateConditionalAssignError(condTok, assignTok, isRedundant); + duplicateConditionalAssignError(condTok, assignTok, isRedundant, isBoolVar); } } } -void CheckConditionImpl::duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant) +void CheckConditionImpl::duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant, bool isBoolVar) { ErrorPath errors; std::string msg = "Duplicate expression for the condition and assignment."; if (condTok && assignTok) { - if (condTok->str() == "==") { + if (condTok->str() == "==" && !isBoolVar) { msg = "Assignment '" + assignTok->expressionString() + "' is redundant with condition '" + condTok->expressionString() + "'."; errors.emplace_back(condTok, "Condition '" + condTok->expressionString() + "'"); errors.emplace_back(assignTok, "Assignment '" + assignTok->expressionString() + "' is redundant"); @@ -1924,7 +1947,6 @@ void CheckConditionImpl::duplicateConditionalAssignError(const Token *condTok, c std::move(errors), Severity::style, "duplicateConditionalAssign", msg, CWE398, Certainty::normal); } - void CheckConditionImpl::checkAssignmentInCondition() { if (!mSettings.severity.isEnabled(Severity::style) && !mSettings.isPremiumEnabled("assignmentInCondition")) diff --git a/lib/checkcondition.h b/lib/checkcondition.h index 232b2b5b340..4a4e1f22db1 100644 --- a/lib/checkcondition.h +++ b/lib/checkcondition.h @@ -176,7 +176,7 @@ class CPPCHECKLIB CheckConditionImpl : public CheckImpl { void invalidTestForOverflow(const Token* tok, const ValueType *valueType, const std::string &replace); void pointerAdditionResultNotNullError(const Token *tok, const Token *calc); - void duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant = false); + void duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant = false, bool isBoolVar = false); void assignmentInCondition(const Token *eq); diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 6d62b644043..5d57ad3e312 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -6406,6 +6406,50 @@ class TestCondition : public TestFixture { "[test.cpp:7:19]: note: Assignment 'b=false'\n" "[test.cpp:7:13]: note: Condition '!b' is redundant\n", errout_str()); + + check("void f(bool& b) {\n" // #14915 + " if (b == true)\n" + " b = false;\n" + "}\n" + "void g(bool& b) {\n" + " if (b == false)\n" + " b = false;\n" + "}\n" + "void h(bool& b) {\n" + " if (b != true)\n" + " b = false;\n" + "}\n" + "void i(bool& b) {\n" + " if (b != false)\n" + " b = false;\n" + "}\n" + "void j(bool& b) {\n" + " if (b == true)\n" + " b = true;\n" + "}\n" + "void k(bool& b) {\n" + " if (true == b)\n" + " b = false;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2:11]: style: The statement 'if (b==true) b=false' is logically equivalent to 'b=false'. [duplicateConditionalAssign]\n" + "[test.cpp:3:11]: note: Assignment 'b=false'\n" + "[test.cpp:2:11]: note: Condition 'b==true' is redundant\n" + "[test.cpp:6:11]: style: The statement 'if (b==false) b=false' is redundant. [duplicateConditionalAssign]\n" + "[test.cpp:7:11]: note: Assignment 'b=false'\n" + "[test.cpp:6:11]: note: Condition 'b==false' is redundant\n" + "[test.cpp:10:11]: style: The statement 'if (b!=true) b=false' is redundant. [duplicateConditionalAssign]\n" + "[test.cpp:11:11]: note: Assignment 'b=false'\n" + "[test.cpp:10:11]: note: Condition 'b!=true' is redundant\n" + "[test.cpp:14:11]: style: The statement 'if (b!=false) b=false' is logically equivalent to 'b=false'. [duplicateConditionalAssign]\n" + "[test.cpp:15:11]: note: Assignment 'b=false'\n" + "[test.cpp:14:11]: note: Condition 'b!=false' is redundant\n" + "[test.cpp:18:11]: style: The statement 'if (b==true) b=true' is redundant. [duplicateConditionalAssign]\n" + "[test.cpp:19:11]: note: Assignment 'b=true'\n" + "[test.cpp:18:11]: note: Condition 'b==true' is redundant\n" + "[test.cpp:22:14]: style: The statement 'if (true==b) b=false' is logically equivalent to 'b=false'. [duplicateConditionalAssign]\n" + "[test.cpp:23:11]: note: Assignment 'b=false'\n" + "[test.cpp:22:14]: note: Condition 'true==b' is redundant\n", + errout_str()); } void checkAssignmentInCondition() {