diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 4b7761510..66474cb1d 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -1537,3 +1537,53 @@ void CheckCondition::pointerAdditionResultNotNullError(const Token *tok, const T const std::string s = calc ? calc->expressionString() : "ptr+1"; 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."); } + +void CheckCondition::checkDuplicateConditionalAssign() +{ + if (!mSettings->isEnabled(Settings::STYLE)) + return; + + const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope *scope : symbolDatabase->functionScopes) { + for (const Token *tok = scope->bodyStart; tok != scope->bodyEnd; tok = tok->next()) { + if (!Token::simpleMatch(tok, "if (")) + continue; + if (!Token::simpleMatch(tok->next()->link(), ") {")) + continue; + const Token *blockTok = tok->next()->link()->next(); + const Token *condTok = tok->next()->astOperand2(); + if (!Token::Match(condTok, "==|!=")) + continue; + if (condTok->str() == "!=" && Token::simpleMatch(blockTok->link(), "} else {")) + continue; + if (!blockTok->next()) + continue; + const Token *assignTok = blockTok->next()->astTop(); + if (!Token::simpleMatch(assignTok, "=")) + 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); + } + } +} + +void CheckCondition::duplicateConditionalAssignError(const Token *condTok, const Token* assignTok) +{ + ErrorPath errors; + if (condTok && assignTok) { + if (condTok->str() == "==") { + errors.emplace_back(condTok, "Condition '" + condTok->expressionString() + "'"); + errors.emplace_back(assignTok, "Assignment '" + assignTok->expressionString() + "' is redundant"); + } else { + errors.emplace_back(assignTok, "Assignment '" + assignTok->expressionString() + "'"); + errors.emplace_back(condTok, "Condition '" + condTok->expressionString() + "' is redundant"); + } + } + reportError( + errors, Severity::style, "duplicateConditionalAssign", "Duplicate expression for the condition and assignment.", CWE398, false); +} diff --git a/lib/checkcondition.h b/lib/checkcondition.h index 421ac0688..bbbf04bdb 100644 --- a/lib/checkcondition.h +++ b/lib/checkcondition.h @@ -61,6 +61,7 @@ public: checkCondition.alwaysTrueFalse(); checkCondition.duplicateCondition(); checkCondition.checkPointerAdditionResultNotNull(); + checkCondition.checkDuplicateConditionalAssign(); checkCondition.assignIf(); checkCondition.checkBadBitmaskCheck(); checkCondition.comparison(); @@ -115,7 +116,9 @@ public: /** @brief Check if pointer addition result is NULL '(ptr + 1) == NULL' */ void checkPointerAdditionResultNotNull(); -private: + void checkDuplicateConditionalAssign(); + + private: // The conditions that have been diagnosed std::set mCondDiags; bool diag(const Token* tok, bool insert=true); @@ -151,6 +154,8 @@ private: void invalidTestForOverflow(const Token* tok, bool result); void pointerAdditionResultNotNullError(const Token *tok, const Token *calc); + void duplicateConditionalAssignError(const Token *condTok, const Token* assignTok); + void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const OVERRIDE { CheckCondition c(nullptr, settings, errorLogger); @@ -172,6 +177,7 @@ private: c.alwaysTrueFalseError(nullptr, nullptr); c.invalidTestForOverflow(nullptr, false); c.pointerAdditionResultNotNullError(nullptr, nullptr); + c.duplicateConditionalAssignError(nullptr, nullptr); } static std::string myName() { @@ -183,6 +189,7 @@ private: "- Mismatching assignment and comparison => comparison is always true/false\n" "- Mismatching lhs and rhs in comparison => comparison is always true/false\n" "- Detect usage of | where & should be used\n" + "- Duplicate condition and assignment\n" "- Detect matching 'if' and 'else if' conditions\n" "- Mismatching bitand (a &= 0xf0; a &= 1; => a = 0)\n" "- Opposite inner condition is always false\n" diff --git a/test/testcondition.cpp b/test/testcondition.cpp index e9e8bce76..b858413e2 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -114,6 +114,7 @@ private: TEST_CASE(checkConditionIsAlwaysTrueOrFalseInsideIfWhile); TEST_CASE(alwaysTrueFalseInLogicalOperators); TEST_CASE(pointerAdditionResultNotNull); + TEST_CASE(duplicateConditionalAssign); } void check(const char code[], const char* filename = "test.cpp", bool inconclusive = false) { @@ -3068,6 +3069,43 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison is wrong. Result of 'ptr+1' can't be 0 unless there is pointer overflow, and pointer overflow is undefined behaviour.\n", errout.str()); } + + void duplicateConditionalAssign() + { + check("void f(int& x, int y) {\n" + " if (x == y)\n" + " x = y;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Duplicate expression for the condition and assignment.\n", errout.str()); + + check("void f(int& x, int y) {\n" + " if (x != y)\n" + " x = y;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:2]: (style) Duplicate expression for the condition and assignment.\n", errout.str()); + + check("void f(int& x, int y) {\n" + " if (x == y)\n" + " x = y;\n" + " else\n" + " x = 1;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Duplicate expression for the condition and assignment.\n", errout.str()); + + check("void f(int& x, int y) {\n" + " if (x != y)\n" + " x = y;\n" + " else\n" + " x = 1;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int& x, int y) {\n" + " if (x == y)\n" + " x = y + 1;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestCondition)