diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 8cb552904..3d3e2022d 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -612,6 +612,7 @@ void CheckOther::checkRedundantAssignment() std::map varAssignments; std::map memAssignments; + std::set initialized; const Token* writtenArgumentsEnd = 0; for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { @@ -628,6 +629,17 @@ void CheckOther::checkRedundantAssignment() varAssignments.clear(); memAssignments.clear(); } else if (tok->type() == Token::eVariable) { + // Set initialization flag + if (!Token::Match(tok, "%var% [")) + initialized.insert(tok->varId()); + else { + const Token *tok2 = tok->next(); + while (tok2 && tok2->str() == "[") + tok2 = tok2->link()->next(); + if (tok2 && tok2->str() != ";") + initialized.insert(tok->varId()); + } + std::map::iterator it = varAssignments.find(tok->varId()); if (tok->next()->isAssignmentOp() && Token::Match(tok->previous(), "[;{}]")) { // Assignment if (it != varAssignments.end()) { @@ -652,7 +664,9 @@ void CheckOther::checkRedundantAssignment() } it->second = tok; } - if (!Token::simpleMatch(tok->tokAt(2), "0 ;") || (tok->variable() && tok->variable()->nameToken() != tok->tokAt(-2))) + if (Token::simpleMatch(tok->tokAt(2), "0 ;")) + varAssignments.erase(tok->varId()); + else varAssignments[tok->varId()] = tok; memAssignments.erase(tok->varId()); } else if (tok->next()->type() == Token::eIncDecOp || (tok->previous()->type() == Token::eIncDecOp && !Token::Match(tok->next(), ".|[|("))) { // Variable incremented/decremented @@ -670,10 +684,12 @@ void CheckOther::checkRedundantAssignment() const Token* param1 = tok->tokAt(2); writtenArgumentsEnd = param1->next(); if (param1->varId() && param1->strAt(1) == "," && !Token::Match(tok, "strcat|strncat|wcscat|wcsncat")) { - std::map::iterator it = memAssignments.find(param1->varId()); - if (it == memAssignments.end()) + if (tok->str() == "memset" && initialized.find(param1->varId()) == initialized.end() && param1->variable() && param1->variable()->isLocal() && param1->variable()->isArray()) + initialized.insert(param1->varId()); + else if (memAssignments.find(param1->varId()) == memAssignments.end()) memAssignments[param1->varId()] = tok; else { + const std::map::iterator it = memAssignments.find(param1->varId()); if (scope->type == Scope::eSwitch && Token::findmatch(it->second, "default|case", tok) && warning) redundantCopyInSwitchError(it->second, tok, param1->str()); else if (performance) @@ -1154,46 +1170,6 @@ void CheckOther::selfAssignmentError(const Token *tok, const std::string &varnam "selfAssignment", "Redundant assignment of '" + varname + "' to itself."); } -//--------------------------------------------------------------------------- -// int a = 1; -// assert(a = 2); // <- assert should not have a side-effect -//--------------------------------------------------------------------------- -static inline const Token *findAssertPattern(const Token *start) -{ - return Token::findmatch(start, "assert ( %any%"); -} - -void CheckOther::checkAssignmentInAssert() -{ - if (!_settings->isEnabled("warning")) - return; - - const Token *tok = findAssertPattern(_tokenizer->tokens()); - const Token *endTok = tok ? tok->next()->link() : NULL; - - while (tok && endTok) { - for (tok = tok->tokAt(2); tok != endTok; tok = tok->next()) { - if (tok->isName() && (tok->next()->isAssignmentOp() || tok->next()->type() == Token::eIncDecOp)) - assignmentInAssertError(tok, tok->str()); - else if (Token::Match(tok, "--|++ %var%")) - assignmentInAssertError(tok, tok->strAt(1)); - } - - tok = findAssertPattern(endTok->next()); - endTok = tok ? tok->next()->link() : NULL; - } -} - -void CheckOther::assignmentInAssertError(const Token *tok, const std::string &varname) -{ - reportError(tok, Severity::warning, - "assignmentInAssert", "Assert statement modifies '" + varname + "'.\n" - "Variable '" + varname + "' is modified insert assert statement. " - "Assert statements are removed from release builds so the code inside " - "assert statement is not executed. If the code is needed also in release " - "builds, this is a bug."); -} - //--------------------------------------------------------------------------- // if ((x != 1) || (x != 3)) // expression always true // if ((x == 1) && (x == 3)) // expression always false diff --git a/test/testother.cpp b/test/testother.cpp index e2a2c2604..b9f52a91d 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -106,9 +106,6 @@ private: TEST_CASE(trac2071); TEST_CASE(trac2084); TEST_CASE(trac3693); - - TEST_CASE(assignmentInAssert); - TEST_CASE(modulo); TEST_CASE(incorrectLogicOperator1); @@ -1759,6 +1756,12 @@ private: " }\n" "}", 0, false, false, false, false); ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" // Ticket #4356 + " int x = 0;\n" // <- ignore assignment with 0 + " x = 3;\n" + "}", 0, false, false, false, false); + ASSERT_EQUALS("", errout.str()); } void switchRedundantOperationTest() { @@ -3229,62 +3232,6 @@ private: ASSERT_EQUALS("", errout.str()); } - void assignmentInAssert() { - check("void f() {\n" - " int a; a = 0;\n" - " assert(a = 2);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); - - check("void f(int a) {\n" - " assert(a == 2);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("", errout.str()); - - check("void f(int a, int b) {\n" - " assert(a == 2 && b = 1);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:2]: (warning) Assert statement modifies 'b'.\n", errout.str()); - - check("void f() {\n" - " int a; a = 0;\n" - " assert(a += 2);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); - - check("void f() {\n" - " int a; a = 0;\n" - " assert(a *= 2);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); - - check("void f() {\n" - " int a; a = 0;\n" - " assert(a -= 2);\n" - " return a;\n" - "}\n" - ); - ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); - - check("void f() {\n" - " int a = 0;\n" - " assert(a--);\n" // don't simplify and verify - " return a;\n" - "}\n", 0, false, false, false, false - ); - ASSERT_EQUALS("[test.cpp:3]: (warning) Assert statement modifies 'a'.\n", errout.str()); - } - void modulo() { check("bool f(bool& b1, bool& b2, bool& b3) {\n" " b1 = a % 5 == 4;\n" @@ -5637,8 +5584,7 @@ private: " memcpy(a, b, 5);\n" " memmove(a, b, 5);\n" "}"); - ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (performance) Buffer 'a' is being written before its old content has been used.\n" - "[test.cpp:3] -> [test.cpp:5]: (performance) Buffer 'a' is being written before its old content has been used.\n" + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5]: (performance) Buffer 'a' is being written before its old content has been used.\n" "[test.cpp:3]: (warning, inconclusive) Array 'a' is filled incompletely. Did you forget to multiply the size given to 'memset()' with 'sizeof(*a)'?\n" "[test.cpp:4]: (warning, inconclusive) Array 'a' is filled incompletely. Did you forget to multiply the size given to 'memcpy()' with 'sizeof(*a)'?\n" "[test.cpp:5]: (warning, inconclusive) Array 'a' is filled incompletely. Did you forget to multiply the size given to 'memmove()' with 'sizeof(*a)'?\n", errout.str()); @@ -5857,24 +5803,6 @@ private: " return x + 1;\n" "}"); ASSERT_EQUALS("", errout.str()); - - check("void f() {\n" // Ticket #4356 - " int x = 0;\n" // <- ignore assignment with 0 - " x = 3;\n" - "}", 0, false, false, false, false); - ASSERT_EQUALS("", errout.str()); - - check("void f() {\n" - " int i = 54;\n" - " i = 0;\n" - "}", 0, false, false, false, false); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (performance) Variable 'i' is reassigned a value before the old one has been used.\n", errout.str()); - - check("void f() {\n" - " int i = 54;\n" - " i = 1;\n" - "}", 0, false, false, false, false); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (performance) Variable 'i' is reassigned a value before the old one has been used.\n", errout.str()); } void redundantMemWrite() { @@ -5961,6 +5889,21 @@ private: " memset(a, 0, size);\n" "}"); ASSERT_EQUALS("", errout.str()); + + // #4455 - initialization of local buffer + check("void f(void) {" + " char buf[10];\n" + " memset(buf, 0, 10);\n" + " strcpy(buf, string);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(void) {\n" + " char buf[10] = {0};\n" + " memset(buf, 0, 10);\n" + " strcpy(buf, string);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (performance) Buffer 'buf' is being written before its old content has been used.\n", errout.str()); } void varFuncNullUB() { // #4482 @@ -6222,4 +6165,4 @@ private: } }; -REGISTER_TEST(TestOther) +REGISTER_TEST(TestOther) \ No newline at end of file