From 0a104c40b7cb4fc6fdd8ace262a1d1861db90a4f Mon Sep 17 00:00:00 2001 From: PKEuS Date: Thu, 9 May 2013 15:25:36 +0200 Subject: [PATCH] Fixed "Improved handling of 0 initializations (#4604)" This fixes commit 1201e417ec568fde523a6a751415eb06778b833a. --- lib/checkother.cpp | 67 +++++++++++++------- test/testother.cpp | 154 +++++++++++++++++++++++++++++---------------- 2 files changed, 147 insertions(+), 74 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index bb1cba53f..8cb552904 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -612,7 +612,6 @@ 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()) { @@ -629,17 +628,6 @@ 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()) { @@ -657,15 +645,14 @@ void CheckOther::checkRedundantAssignment() } } if (error) { - if (scope->typ!Token::simpleMatch(tok->tokAt(2), "0 ;") || (tok->variable() && tok->variable()->nameToken() != tok->tokAt(-2)))AssignmentInSwitchError(it->second, tok, tok->str()); + if (scope->type == Scope::eSwitch && Token::findmatch(it->second, "default|case", tok) && warning) + redundantAssignmentInSwitchError(it->second, tok, tok->str()); else if (performance) redundantAssignmentError(it->second, tok, tok->str(), nonLocal(it->second->variable())); // Inconclusive for non-local variables } it->second = tok; } - if (Token::simpleMatch(tok->tokAt(2), "0 ;")) - varAssignments.erase(tok->varId()); - else + if (!Token::simpleMatch(tok->tokAt(2), "0 ;") || (tok->variable() && tok->variable()->nameToken() != tok->tokAt(-2))) 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 @@ -683,12 +670,10 @@ 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")) { - 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()) + std::map::iterator it = memAssignments.find(param1->varId()); + if (it == 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) @@ -1169,6 +1154,46 @@ 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 @@ -2118,7 +2143,7 @@ void CheckOther::checkMathFunctions() const std::size_t functions = symbolDatabase->functionScopes.size(); for (std::size_t i = 0; i < functions; ++i) { const Scope * scope = symbolDatabase->functionScopes[i]; - ch(tok, "div|st Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { + for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) { if (tok->varId()) continue; if (Token::Match(tok, "log|log10 ( %num% )")) { diff --git a/test/testother.cpp b/test/testother.cpp index bc2e9ffd4..e2a2c2604 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -106,6 +106,9 @@ private: TEST_CASE(trac2071); TEST_CASE(trac2084); TEST_CASE(trac3693); + + TEST_CASE(assignmentInAssert); + TEST_CASE(modulo); TEST_CASE(incorrectLogicOperator1); @@ -1756,17 +1759,18 @@ private: " }\n" "}", 0, false, false, false, false); ASSERT_EQUALS("", errout.str()); + } - check("void f() {\n" // Ticket } - - void switchRedundantOperationTest() { "{\n" - " std::cout << log(-0.1) << std::endl;\n" -a)\n" + void switchRedundantOperationTest() { + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" " {\n" " case 2:\n" - " {\n" " ++y;\n" - " y = 3;\n" + " case 3:\n" + " y = 3;\n" " }\n" " bar(y);\n" "}"); @@ -2932,7 +2936,7 @@ a)\n" "{\n" " std::string var = var = \"test\";\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'var' to itself.\n", errout.ssignment of 'var' to itself.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (warning) Redundant assignment of 'var' to itself.\n", errout.str()); // #4073 (segmentation fault) check("void Foo::myFunc( int a )\n" @@ -2940,7 +2944,11 @@ a)\n" " if (a == 42)\n" " a = a;\n" "}"); -ning, inconclusive) Array 'a' is filled incompletely. Did you forget x = x + 1;\n" + + check("void foo()\n" + "{\n" + " int x = 1;\n" + " x = x + 1;\n" " return 0;\n" "}"); ASSERT_EQUALS("", errout.str()); @@ -3221,6 +3229,62 @@ ning, inconclusive) Array 'a' is filled incompletely. Did you forget x = x + 1;\ 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" @@ -5573,7 +5637,8 @@ ning, inconclusive) Array 'a' is filled incompletely. Did you forget x = x + 1;\ " memcpy(a, b, 5);\n" " memmove(a, b, 5);\n" "}"); - ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5]: (performance) Buffer 'a' is being written before its old content has been used.\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" "[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()); @@ -5781,21 +5846,35 @@ ning, inconclusive) Array 'a' is filled incompletely. Did you forget x = x + 1;\ " x = 1;\n" " return x + 1;\n" "}", NULL, false, false, false, false); - ASSERT_EQUALS("[test.cpp:3] -> [test. " i = 1;\n" - " bar();\n" - " i = 1;\n" - "}"); - ASS// #4513 - check("int x;\n" - "int g() {\n" - " return x*x;\n" - "}\n" - x = 1;\n" + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (performance) Variable 'x' is reassigned a value before the old one has been used.\n", errout.str()); + + // from #3103 (avoid a false positive) + check("int foo(){\n" + " int x;\n" + " x = 1;\n" " if (y)\n" // <-- cppcheck does not know anything about 'y' " x = 2;\n" " 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() { @@ -5847,23 +5926,7 @@ ning, inconclusive) Array 'a' is filled incompletely. Did you forget x = x + 1;\ check("void f(void* a) {\n" " snprintf(a, foo, bar);\n" " bar();\n" - " mems - check("void f() {\n" // Ticket #4356 - " int x = 0;\n" // <- ignore assignment with 0 - " x = 3;\n" - "}", 0, false, false, false, falseerrout.str()); - - - // check fgetc - check("voif f (FILE * pFile){\n" - int i = 54;\n" - " i = 0;\n" - "}", 0, false, false, false, false); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (perfocpp:4]: (performance) Variable 'x' is reassigned a value before the old one has been used.\n", errout.strcheck("void f() {\n" - " int i = 54;\n" - " i = 1;\n" - "}", 0, false, false, false, false); - ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (perfocpp:4]: (performance) Variable 'x' is reassigned a value before the old one has been used.\n", eet(a, 0, size);\n" + " memset(a, 0, size);\n" "}"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:4]: (performance) Buffer 'a' is being written before its old content has been used.\n", errout.str()); @@ -5898,21 +5961,6 @@ ning, inconclusive) Array 'a' is filled incompletely. Did you forget x = x + 1;\ " 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 @@ -6174,4 +6222,4 @@ ning, inconclusive) Array 'a' is filled incompletely. Did you forget x = x + 1;\ } }; -REGISTER_TEST(TestOther) \ No newline at end of file +REGISTER_TEST(TestOther)