From dcb332acb0b098d4f803d84b878cd48510310709 Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Tue, 20 Sep 2022 07:30:12 +0200 Subject: [PATCH] Fix #9406 FN redundant assignment of Boolean variable (#4482) * Fix #9406 FN redundant assignment of Boolean variable * Fix warning, refactor * Update testcondition.cpp --- lib/checkcondition.cpp | 39 ++++++++++++++++++++++++++---------- lib/checkcondition.h | 2 +- lib/checkunusedfunctions.cpp | 4 +--- test/testcondition.cpp | 39 +++++++++++++++++++++++++++++++++--- 4 files changed, 66 insertions(+), 18 deletions(-) diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 7637689c8..5d0e77957 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1750,9 +1750,10 @@ void CheckCondition::checkDuplicateConditionalAssign() continue; const Token *blockTok = tok->next()->link()->next(); const Token *condTok = tok->next()->astOperand2(); - if (!Token::Match(condTok, "==|!=")) + const bool isBoolVar = Token::Match(condTok, "!| %var%"); + if (!isBoolVar && !Token::Match(condTok, "==|!=")) continue; - if (condTok->str() == "!=" && Token::simpleMatch(blockTok->link(), "} else {")) + if ((isBoolVar || condTok->str() == "!=") && Token::simpleMatch(blockTok->link(), "} else {")) continue; if (!blockTok->next()) continue; @@ -1761,18 +1762,33 @@ void CheckCondition::checkDuplicateConditionalAssign() continue; if (nextAfterAstRightmostLeaf(assignTok) != blockTok->link()->previous()) continue; - if (!isSameExpression( - mTokenizer->isCPP(), true, condTok->astOperand1(), assignTok->astOperand1(), mSettings->library, true, true)) - continue; - if (!isSameExpression( - mTokenizer->isCPP(), true, condTok->astOperand2(), assignTok->astOperand2(), mSettings->library, true, true)) - continue; - duplicateConditionalAssignError(condTok, assignTok); + bool isRedundant = false; + if (isBoolVar) { + if (!astIsBool(condTok)) + continue; + const bool isNegation = condTok->str() == "!"; + if (!(assignTok->astOperand1() && assignTok->astOperand1()->varId() == (isNegation ? condTok->next() : condTok)->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); + } else { // comparison + if (!isSameExpression( + mTokenizer->isCPP(), true, condTok->astOperand1(), assignTok->astOperand1(), mSettings->library, true, true)) + continue; + if (!isSameExpression( + mTokenizer->isCPP(), true, condTok->astOperand2(), assignTok->astOperand2(), mSettings->library, true, true)) + continue; + } + duplicateConditionalAssignError(condTok, assignTok, isRedundant); } } } -void CheckCondition::duplicateConditionalAssignError(const Token *condTok, const Token* assignTok) +void CheckCondition::duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant) { ErrorPath errors; std::string msg = "Duplicate expression for the condition and assignment."; @@ -1782,7 +1798,8 @@ void CheckCondition::duplicateConditionalAssignError(const Token *condTok, const errors.emplace_back(condTok, "Condition '" + condTok->expressionString() + "'"); errors.emplace_back(assignTok, "Assignment '" + assignTok->expressionString() + "' is redundant"); } else { - msg = "The statement 'if (" + condTok->expressionString() + ") " + assignTok->expressionString() + "' is logically equivalent to '" + assignTok->expressionString() + "'."; + msg = "The statement 'if (" + condTok->expressionString() + ") " + assignTok->expressionString(); + msg += isRedundant ? "' is redundant." : "' is logically equivalent to '" + assignTok->expressionString() + "'."; errors.emplace_back(assignTok, "Assignment '" + assignTok->expressionString() + "'"); errors.emplace_back(condTok, "Condition '" + condTok->expressionString() + "' is redundant"); } diff --git a/lib/checkcondition.h b/lib/checkcondition.h index b1273f8f1..fa894b728 100644 --- a/lib/checkcondition.h +++ b/lib/checkcondition.h @@ -165,7 +165,7 @@ private: 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); + void duplicateConditionalAssignError(const Token *condTok, const Token* assignTok, bool isRedundant = false); void assignmentInCondition(const Token *eq); diff --git a/lib/checkunusedfunctions.cpp b/lib/checkunusedfunctions.cpp index 444086031..fd156175b 100644 --- a/lib/checkunusedfunctions.cpp +++ b/lib/checkunusedfunctions.cpp @@ -120,9 +120,7 @@ void CheckUnusedFunctions::parseTokens(const Tokenizer &tokenizer, const char Fi while ((scope || start) && markupVarToken) { if (markupVarToken->str() == settings->library.blockstart(FileName)) { scope++; - if (start) { - start = false; - } + start = false; } else if (markupVarToken->str() == settings->library.blockend(FileName)) scope--; else if (!settings->library.iskeyword(FileName, markupVarToken->str())) { diff --git a/test/testcondition.cpp b/test/testcondition.cpp index f810d55ea..a1152b212 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -4073,21 +4073,21 @@ private: " if (init)\n" " init = false;\n" "}\n"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) The statement 'if (init) init=false' is logically equivalent to 'init=false'.\n", errout.str()); check("void f() {\n" " static bool init(true);\n" " if (init)\n" " init = false;\n" "}\n"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) The statement 'if (init) init=false' is logically equivalent to 'init=false'.\n", errout.str()); check("void f() {\n" " static bool init{ true };\n" " if (init)\n" " init = false;\n" "}\n"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:3]: (style) The statement 'if (init) init=false' is logically equivalent to 'init=false'.\n", errout.str()); // #10248 check("void f() {\n" @@ -5431,6 +5431,39 @@ private: " }\n" "}"); ASSERT_EQUALS("", errout.str()); + + check("bool f(bool b) {\n" + " if (b)\n" + " b = false;\n" + " else\n" + " g();\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("struct S {\n" // #9406 + " S() : b(false) {}\n" + " void f() {\n" + " if (b) b = true;\n" + " if (b) b = false;\n" + " if (!b) b = true;\n" + " if (!b) b = false;\n" + " }\n" + " bool b;\n" + "};\n"); + ASSERT_EQUALS("test.cpp:4:style:The statement 'if (b) b=true' is redundant.\n" + "test.cpp:4:note:Assignment 'b=true'\n" + "test.cpp:4:note:Condition 'b' is redundant\n" + "test.cpp:5:style:The statement 'if (b) b=false' is logically equivalent to 'b=false'.\n" + "test.cpp:5:note:Assignment 'b=false'\n" + "test.cpp:5:note:Condition 'b' is redundant\n" + "test.cpp:6:style:The statement 'if (!b) b=true' is logically equivalent to 'b=true'.\n" + "test.cpp:6:note:Assignment 'b=true'\n" + "test.cpp:6:note:Condition '!b' is redundant\n" + "test.cpp:7:style:The statement 'if (!b) b=false' is redundant.\n" + "test.cpp:7:note:Assignment 'b=false'\n" + "test.cpp:7:note:Condition '!b' is redundant\n", + errout.str()); } void checkAssignmentInCondition() {