From 2d64b69cf477a7f70ae11f40e0b26665b1ec2f20 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Sun, 2 Sep 2012 13:09:32 +0200 Subject: [PATCH] New check: Detect redundant assignment to a variable and redundant copying to a buffer This check partially replaces the check for redundant assignments in switch --- lib/checkother.cpp | 227 ++++++++++++++++++++++++-------------- lib/checkother.h | 18 +++- test/testother.cpp | 264 +++++++++++++++++++++++++++++++++++++-------- 3 files changed, 384 insertions(+), 125 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index dff5b6fc8..688bc77a0 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -610,6 +610,155 @@ void CheckOther::sizeofForPointerError(const Token *tok, const std::string &varn "write sizeof(*" + varname + ")", true); } +//--------------------------------------------------------------------------- +// Detect redundant assignments: x = 0; x = 4; +//--------------------------------------------------------------------------- + +void CheckOther::checkRedundantAssignment() +{ + if (!_settings->isEnabled("performance")) + return; + + const SymbolDatabase* symbolDatabase = _tokenizer->getSymbolDatabase(); + + for (std::list::const_iterator i = symbolDatabase->scopeList.begin(); i != symbolDatabase->scopeList.end(); ++i) { + if (!i->isExecutable()) + continue; + + ///std::cout << std::endl << "scope: " << i->className << std::endl; + + std::map varAssignments; + std::map memAssignments; + const Token* writtenArgumentsEnd = 0; + + for (const Token* tok = i->classStart->next(); tok != i->classEnd; tok = tok->next()) { + if (tok == writtenArgumentsEnd) + writtenArgumentsEnd = 0; + + if (tok->str() == "{" && tok->strAt(-1) != "{" && tok->strAt(-1) != "=" && tok->strAt(-4) != "case" && tok->strAt(-3) != "default") { // conditional or non-executable inner scope: Skip it and reset status + tok = tok->link(); + varAssignments.clear(); + memAssignments.clear(); + } else if (Token::Match(tok, "for|if|while (")) { + tok = tok->linkAt(1); + } else if (Token::Match(tok, "break|return|continue|throw|goto")) { + varAssignments.clear(); + memAssignments.clear(); + } else if (tok->type() == Token::eVariable) { + std::map::iterator it = varAssignments.find(tok->varId()); + if (tok->next()->isAssignmentOp() && Token::Match(tok->previous(), "[;{}]")) { // Assignment + ///std::cout << "assign: " << tok->varId() << std::endl; + if (it != varAssignments.end()) { + bool error = true; // Ensure that variable is not used on right side + for (const Token* tok2 = tok->tokAt(2); tok2; tok2 = tok2->next()) { + if (tok2->str() == ";") + break; + else if (tok2->varId() == tok->varId()) + error = false; + } + if (error) { + if (i->type == Scope::eSwitch && Token::findmatch(it->second, "default|case", tok)) + redundantAssignmentInSwitchError(it->second, tok, tok->str()); + else + redundantAssignmentError(it->second, tok, tok->str()); + } + it->second = tok; + } + 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 + varAssignments[tok->varId()] = tok; + memAssignments.erase(tok->varId()); + } else if (!Token::Match(tok->tokAt(-2), "sizeof (")) { // Other usage of variable + ///std::cout << "use: " << tok->varId() << std::endl; + if (it != varAssignments.end()) + varAssignments.erase(it); + if (!writtenArgumentsEnd) // Indicates that we are in the first argument of strcpy/memcpy/... function + memAssignments.erase(tok->varId()); + } + } else if (Token::Match(tok, "%var% (")) { // Function call. Global variables might be used. Reset their status + bool memfunc = Token::Match(tok, "memcpy|memmove|memset|strcpy|strncpy|sprintf|snprintf|strcat|strncat"); + if (memfunc) { + const Token* param1 = tok->tokAt(2); + writtenArgumentsEnd = param1->next(); + if (param1->varId() && param1->strAt(1) == "," && tok->str() != "strcat" && tok->str() != "strncat") { + std::map::iterator it = memAssignments.find(param1->varId()); + if (it == memAssignments.end()) + memAssignments[param1->varId()] = tok; + else { + if (i->type == Scope::eSwitch && Token::findmatch(it->second, "default|case", tok)) + redundantCopyInSwitchError(it->second, tok, param1->str()); + else + redundantCopyError(it->second, tok, param1->str()); + } + } + } else { + const Function* func = symbolDatabase->findFunctionByToken(_tokenizer->getFunctionTokenByName(tok->str().c_str())); + if (!func || !func->hasBody) { + varAssignments.clear(); + memAssignments.clear(); + continue; + } + const Token* funcEnd = func->functionScope->classEnd; + bool noreturn; + if (!_tokenizer->IsScopeNoReturn(funcEnd, &noreturn) && !noreturn) { + for (std::map::iterator i = varAssignments.begin(); i != varAssignments.end(); ++i) { + const Variable* var = symbolDatabase->getVariableFromVarId(i->first); + if (!var || (!var->isLocal() && !var->isArgument())) + i = varAssignments.erase(i); + } + for (std::map::iterator i = memAssignments.begin(); i != memAssignments.end(); ++i) { + const Variable* var = symbolDatabase->getVariableFromVarId(i->first); + if (!var || (!var->isLocal() && !var->isArgument())) + i = memAssignments.erase(i); + } + } else { + varAssignments.clear(); + memAssignments.clear(); + } + } + } + } + } +} + +void CheckOther::redundantCopyError(const Token *tok1, const Token* tok2, const std::string& var) +{ + std::list callstack; + callstack.push_back(tok1); + callstack.push_back(tok2); + reportError(callstack, Severity::performance, "redundantCopy", + "Buffer '" + var + "' is being written before its old content has been used."); +} + +void CheckOther::redundantCopyInSwitchError(const Token *tok1, const Token* tok2, const std::string &var) +{ + std::list callstack; + callstack.push_back(tok1); + callstack.push_back(tok2); + reportError(callstack, Severity::warning, "redundantCopyInSwitch", + "Buffer '" + var + "' is being written before its old content has been used. This might indicate a missing 'break;'."); +} + +void CheckOther::redundantAssignmentError(const Token *tok1, const Token* tok2, const std::string& var) +{ + std::list callstack; + callstack.push_back(tok1); + callstack.push_back(tok2); + reportError(callstack, Severity::performance, "redundantAssignment", + "Variable '" + var + "' is reassigned a value before the old one has been used."); +} + +void CheckOther::redundantAssignmentInSwitchError(const Token *tok1, const Token* tok2, const std::string &var) +{ + std::list callstack; + callstack.push_back(tok1); + callstack.push_back(tok2); + reportError(callstack, Severity::warning, "redundantAssignInSwitch", + "Variable '" + var + "' is reassigned a value before the old one has been used. This might indicate a missing 'break;'."); +} + + //--------------------------------------------------------------------------- // switch (x) // { @@ -636,18 +785,8 @@ void CheckOther::checkRedundantAssignmentInSwitch() continue; // Check the contents of the switch statement - std::map varsAssigned; - std::map stringsCopied; std::map varsWithBitsSet; std::map bitOperations; - /** - * A separate map for variables with post/pre increment/decrement has been kept since - * CASE 1(Reduntant): CASE 2(NOT Reduntant): CASE 3(NOT Reduntant): - * ret++; ret = 3; ret++; - * ret = 2; ret++; ret++; - * - */ - std::map varsOperatedByPostORPreFix; for (const Token *tok2 = i->classStart->next(); tok2 != i->classEnd; tok2 = tok2->next()) { if (tok2->str() == "{") { @@ -658,19 +797,11 @@ void CheckOther::checkRedundantAssignmentInSwitch() const Token* endOfConditional = tok2->link(); for (const Token* tok3 = tok2; tok3 != endOfConditional; tok3 = tok3->next()) { if (tok3->varId() != 0) { - varsAssigned.erase(tok3->varId()); - varsOperatedByPostORPreFix.erase(tok3->varId()); - stringsCopied.erase(tok3->varId()); varsWithBitsSet.erase(tok3->varId()); bitOperations.erase(tok3->varId()); } else if (Token::Match(tok3, functionPattern) || Token::Match(tok3, breakPattern)) { - varsAssigned.clear(); varsWithBitsSet.clear(); bitOperations.clear(); - varsOperatedByPostORPreFix.erase(tok3->varId()); - - if (tok3->str() != "strcpy" && tok3->str() != "strncpy") - stringsCopied.clear(); } } tok2 = endOfConditional; @@ -682,31 +813,10 @@ void CheckOther::checkRedundantAssignmentInSwitch() // case 4: b = 2; if (Token::Match(tok2->previous(), ";|{|}|: %var% = %any% ;") && tok2->varId() != 0) { - std::map::iterator i2 = varsAssigned.find(tok2->varId()); - std::map::iterator i3 = varsOperatedByPostORPreFix.find(tok2->varId()); - - if (i2 == varsAssigned.end() && i3 == varsOperatedByPostORPreFix.end()) - varsAssigned[tok2->varId()] = tok2; - - else { - if (i3 == varsOperatedByPostORPreFix.end()) - redundantAssignmentInSwitchError(i2->second, i2->second->str()); - else - redundantOperationInSwitchError(i3->second, i3->second->str()); - - } - stringsCopied.erase(tok2->varId()); varsWithBitsSet.erase(tok2->varId()); bitOperations.erase(tok2->varId()); } - - else if ((Token::Match(tok2->previous(), ";|{|}|: %var% ++|-- ;") || - Token::Match(tok2->tokAt(-2), ";|{|}|: ++|-- %var% ;")) && tok2->varId() != 0) { - std::map::iterator i2 = varsOperatedByPostORPreFix.find(tok2->varId()); - if (i2 == varsOperatedByPostORPreFix.end()) - varsOperatedByPostORPreFix[tok2->varId()] = tok2; - } // Bitwise operation. Report an error if it's performed twice before a break. E.g.: // case 3: b |= 1; // <== redundant // case 4: b |= 1; @@ -730,26 +840,12 @@ void CheckOther::checkRedundantAssignmentInSwitch() varsWithBitsSet.erase(tok2->varId()); bitOperations.erase(tok2->varId()); } - - stringsCopied.erase(tok2->varId()); - varsAssigned.erase(tok2->varId()); } - // String copy. Report an error if it's copied to twice before a break. E.g.: - // case 3: strcpy(str, "a"); // <== redundant - // case 4: strcpy(str, "b"); - else if (Token::Match(tok2->previous(), ";|{|}|: strcpy|strncpy ( %var% ,") && tok2->tokAt(2)->varId() != 0) { - std::map::iterator i2 = stringsCopied.find(tok2->tokAt(2)->varId()); - if (i2 == stringsCopied.end()) - stringsCopied[tok2->tokAt(2)->varId()] = tok2->tokAt(2); - else - redundantStrcpyInSwitchError(i2->second, i2->second->str()); - } // Not a simple assignment so there may be good reason if this variable is assigned to twice. E.g.: // case 3: b = 1; // case 4: b++; else if (tok2->varId() != 0 && tok2->strAt(1) != "|" && tok2->strAt(1) != "&") { - varsAssigned.erase(tok2->varId()); varsWithBitsSet.erase(tok2->varId()); bitOperations.erase(tok2->varId()); } @@ -757,44 +853,19 @@ void CheckOther::checkRedundantAssignmentInSwitch() // Reset our record of assignments if there is a break or function call. E.g.: // case 3: b = 1; break; if (Token::Match(tok2, functionPattern) || Token::Match(tok2, breakPattern)) { - varsAssigned.clear(); varsWithBitsSet.clear(); bitOperations.clear(); - varsOperatedByPostORPreFix.clear(); - - if (tok2->str() != "strcpy" && tok2->str() != "strncpy") - stringsCopied.clear(); } } } } -void CheckOther::redundantAssignmentInSwitchError(const Token *tok, const std::string &varname) -{ - reportError(tok, Severity::warning, - "redundantAssignInSwitch", "Redundant assignment of \"" + varname + "\" in switch"); -} - -void CheckOther::redundantOperationInSwitchError(const Token *tok, const std::string &varname) -{ - reportError(tok, Severity::warning, - "redundantOperationInSwitch", "Redundant operation on '" + varname + "' in switch."); - -} - void CheckOther::redundantBitwiseOperationInSwitchError(const Token *tok, const std::string &varname) { reportError(tok, Severity::warning, "redundantBitwiseOperationInSwitch", "Redundant bitwise operation on \"" + varname + "\" in switch"); } -void CheckOther::redundantStrcpyInSwitchError(const Token *tok, const std::string &varname) -{ - reportError(tok, Severity::warning, - "redundantStrcpyInSwitch", - "Switch case fall-through. Redundant strcpy of \"" + varname + "\".\n" - "Switch case fall-through. Redundant strcpy of \"" + varname + "\". The string is overwritten in a later case block."); -} //--------------------------------------------------------------------------- //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index e8789923e..bed34b1e6 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -59,6 +59,7 @@ public: checkOther.strPlusChar(); checkOther.sizeofsizeof(); checkOther.sizeofCalculation(); + checkOther.checkRedundantAssignment(); checkOther.checkRedundantAssignmentInSwitch(); checkOther.checkAssignmentInAssert(); checkOther.checkSizeofForArrayParameter(); @@ -168,6 +169,9 @@ public: /** @brief %Check for calculations inside sizeof */ void sizeofCalculation(); + /** @brief copying to memory or assigning to a variablen twice */ + void checkRedundantAssignment(); + /** @brief %Check for assigning to the same variable twice in a switch statement*/ void checkRedundantAssignmentInSwitch(); @@ -277,10 +281,11 @@ private: void zerodivError(const Token *tok); void mathfunctionCallError(const Token *tok, const unsigned int numParam = 1); void cctypefunctionCallError(const Token *tok, const std::string &functionName, const std::string &value); - void redundantAssignmentInSwitchError(const Token *tok, const std::string &varname); - void redundantOperationInSwitchError(const Token *tok, const std::string &varname); + void redundantAssignmentError(const Token *tok1, const Token* tok2, const std::string& var); + void redundantAssignmentInSwitchError(const Token *tok1, const Token *tok2, const std::string &var); + void redundantCopyError(const Token *tok1, const Token* tok2, const std::string& var); + void redundantCopyInSwitchError(const Token *tok1, const Token* tok2, const std::string &var); void redundantBitwiseOperationInSwitchError(const Token *tok, const std::string &varname); - void redundantStrcpyInSwitchError(const Token *tok, const std::string &varname); void switchCaseFallThrough(const Token *tok); void selfAssignmentError(const Token *tok, const std::string &varname); void assignmentInAssertError(const Token *tok, const std::string &varname); @@ -337,6 +342,8 @@ private: //performance c.redundantCopyError(0, "varname"); + c.redundantCopyError(0, 0, "var"); + c.redundantAssignmentError(0, 0, "var"); // style/warning c.cstyleCastError(0); @@ -349,8 +356,8 @@ private: c.strPlusCharError(0); c.sizeofsizeofError(0); c.sizeofCalculationError(0, false); - c.redundantAssignmentInSwitchError(0, "varname"); - c.redundantOperationInSwitchError(0, "varname"); + c.redundantAssignmentInSwitchError(0, 0, "var"); + c.redundantCopyInSwitchError(0, 0, "var"); c.switchCaseFallThrough(0); c.selfAssignmentError(0, "varname"); c.assignmentInAssertError(0, "varname"); @@ -405,6 +412,7 @@ private: //performance "* redundant data copying for const variable\n" + "* subsequent assignment or copying to a variable or buffer\n" // style "* C-style pointer cast in cpp file\n" diff --git a/test/testother.cpp b/test/testother.cpp index 50c78415d..622842f01 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -168,6 +168,9 @@ private: TEST_CASE(checkNegativeShift); TEST_CASE(incompleteArrayFill); + + TEST_CASE(redundantVarAssignment); + TEST_CASE(redundantMemWrite); } void check(const char code[], const char *filename = NULL, bool experimental = false, bool inconclusive = true) { @@ -177,6 +180,7 @@ private: Settings settings; settings.addEnabled("style"); settings.addEnabled("portability"); + settings.addEnabled("performance"); settings.inconclusive = inconclusive; settings.experimental = experimental; @@ -1384,7 +1388,7 @@ private: " }\n" " bar(y);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:7]: (warning) Redundant assignment of \"y\" in switch\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:9]: (warning) Variable 'y' is reassigned a value before the old one has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo()\n" "{\n" @@ -1400,7 +1404,7 @@ private: " }\n" " bar(y);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:8]: (warning) Redundant assignment of \"y\" in switch\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:11]: (warning) Variable 'y' is reassigned a value before the old one has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo()\n" "{\n" @@ -1557,7 +1561,7 @@ private: " strcpy(str, \"b'\");\n" " }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:6]: (warning) Switch case fall-through. Redundant strcpy of \"str\".\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:8]: (warning) Buffer 'str' is being written before its old content has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo(char *str, int a)\n" "{\n" @@ -1569,7 +1573,7 @@ private: " strncpy(str, \"b'\");\n" " }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:6]: (warning) Switch case fall-through. Redundant strcpy of \"str\".\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6] -> [test.cpp:8]: (warning) Buffer 'str' is being written before its old content has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo(char *str, int a)\n" "{\n" @@ -1584,7 +1588,7 @@ private: " z++;\n" " }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:7]: (warning) Switch case fall-through. Redundant strcpy of \"str\".\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:10]: (warning) Buffer 'str' is being written before its old content has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo(char *str, int a)\n" "{\n" @@ -1615,7 +1619,6 @@ private: } void switchRedundantOperationTest() { - check("void foo()\n" "{\n" " int y = 1;\n" @@ -1628,7 +1631,7 @@ private: " }\n" " bar(y);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:7]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:9]: (warning) Variable 'y' is reassigned a value before the old one has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo()\n" "{\n" " int y = 1;\n" @@ -1643,7 +1646,7 @@ private: " }\n" " bar(y);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:8]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:11]: (warning) Variable 'y' is reassigned a value before the old one has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo()\n" "{\n" " int y = 1;\n" @@ -1682,7 +1685,7 @@ private: " }\n" " bar(y);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:7]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:9]: (warning) Variable 'y' is reassigned a value before the old one has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo()\n" "{\n" " int y = 1;\n" @@ -1697,7 +1700,7 @@ private: " }\n" " bar(y);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:8]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:11]: (warning) Variable 'y' is reassigned a value before the old one has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo()\n" "{\n" " int y = 1;\n" @@ -1736,7 +1739,7 @@ private: " }\n" " bar(y);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:7]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:9]: (warning) Variable 'y' is reassigned a value before the old one has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo()\n" "{\n" " int y = 1;\n" @@ -1751,7 +1754,7 @@ private: " }\n" " bar(y);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:8]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:11]: (warning) Variable 'y' is reassigned a value before the old one has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo()\n" "{\n" " int y = 1;\n" @@ -1790,7 +1793,7 @@ private: " }\n" " bar(y);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:7]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7] -> [test.cpp:9]: (warning) Variable 'y' is reassigned a value before the old one has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo()\n" "{\n" " int y = 1;\n" @@ -1805,7 +1808,7 @@ private: " }\n" " bar(y);\n" "}\n"); - ASSERT_EQUALS("[test.cpp:8]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:8] -> [test.cpp:11]: (warning) Variable 'y' is reassigned a value before the old one has been used. This might indicate a missing 'break;'.\n", errout.str()); check("void foo()\n" "{\n" " int y = 1;\n" @@ -1879,21 +1882,6 @@ private: " bar(y);\n" "}\n"); ASSERT_EQUALS("", errout.str()); - check("void foo()\n" - "{\n" - " int y = 1;\n" - " switch (x)\n" - " {\n" - " case 2:\n" - " {\n" - " int y++;\n" - " }\n" - " case 3:\n" - " y = 3;\n" - " }\n" - " bar(y);\n" - "}\n"); - ASSERT_EQUALS("", errout.str()); check("void foo()\n" "{\n" " int y = 1;\n" @@ -3368,9 +3356,9 @@ private: void incorrectLogicOperator3() { check("void f(int x, bool& b) {\n" " b = x > 5 && x == 1;\n" - " b = x < 1 && x == 3;\n" - " b = x >= 5 && x == 1;\n" - " b = x <= 1 && x == 3;\n" + " c = x < 1 && x == 3;\n" + " d = x >= 5 && x == 1;\n" + " e = x <= 1 && x == 3;\n" "}"); ASSERT_EQUALS("[test.cpp:2]: (warning) Logical conjunction always evaluates to false: x > 5 && x == 1.\n" "[test.cpp:3]: (warning) Logical conjunction always evaluates to false: x < 1 && x == 3.\n" @@ -3409,9 +3397,9 @@ private: check("void f(int x, bool& b) {\n" " b = x > 3 || x == 4;\n" - " b = x < 5 || x == 4;\n" - " b = x >= 3 || x == 4;\n" - " b = x <= 5 || x == 4;\n" + " c = x < 5 || x == 4;\n" + " d = x >= 3 || x == 4;\n" + " e = x <= 5 || x == 4;\n" "}"); ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x == 4, the comparison x > 3 is always true.\n" "[test.cpp:3]: (style) Redundant condition: If x == 4, the comparison x < 5 is always true.\n" @@ -3420,9 +3408,9 @@ private: check("void f(int x, bool& b) {\n" " b = x > 5 || x != 1;\n" - " b = x < 1 || x != 3;\n" - " b = x >= 5 || x != 1;\n" - " b = x <= 1 || x != 3;\n" + " c = x < 1 || x != 3;\n" + " d = x >= 5 || x != 1;\n" + " e = x <= 1 || x != 3;\n" "}"); ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x > 5, the comparison x != 1 is always true.\n" "[test.cpp:3]: (style) Redundant condition: If x < 1, the comparison x != 3 is always true.\n" @@ -3431,9 +3419,9 @@ private: check("void f(int x, bool& b) {\n" " b = x > 6 && x > 5;\n" - " b = x > 5 || x > 6;\n" - " b = x < 6 && x < 5;\n" - " b = x < 5 || x < 6;\n" + " c = x > 5 || x > 6;\n" + " d = x < 6 && x < 5;\n" + " e = x < 5 || x < 6;\n" "}"); ASSERT_EQUALS("[test.cpp:2]: (style) Redundant condition: If x > 6, the comparison x > 5 is always true.\n" "[test.cpp:3]: (style) Redundant condition: If x > 5, the comparison x > 6 is always true.\n" @@ -5963,7 +5951,9 @@ private: " memcpy(a, b, 5);\n" " memmove(a, b, 5);\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning, inconclusive) Array 'a' is filled incompletely. Did you forget to multiply the size given to 'memset()' with 'sizeof(*a)'?\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()); @@ -6016,6 +6006,196 @@ private: "}"); ASSERT_EQUALS("[test.cpp:3]: (portability, inconclusive) Array 'a' might be filled incompletely. Did you forget to multiply the size given to 'memset()' with 'sizeof(*a)'?\n", errout.str()); } + + void redundantVarAssignment() { + // Simple tests + check("void f(int i) {\n" + " i = 1;\n" + " i = 1;\n" + "}"); + 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("int i;\n" + "void f() {\n" + " i = 1;\n" + " i = 1;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (performance) Variable 'i' is reassigned a value before the old one has been used.\n", errout.str()); + + check("void f() {\n" + " int i;\n" + " i = 1;\n" + " i = 1;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (performance) Variable 'i' is reassigned a value before the old one has been used.\n", errout.str()); + + // Testing different types + check("void f() {\n" + " Foo& bar = foo();\n" + " bar = x;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + check("void f() {\n" + " Foo& bar = foo();\n" + " bar = x;\n" + " bar = y();\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (performance) Variable 'bar' is reassigned a value before the old one has been used.\n", errout.str()); + + // Tests with function call between assignment + check("void bar() {}\n" + "void f(int i) {\n" + " i = 1;\n" + " bar();\n" + " i = 1;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (performance) Variable 'i' is reassigned a value before the old one has been used.\n", errout.str()); + + check("void bar() {}\n" + "int i;\n" + "void f() {\n" + " i = 1;\n" + " bar();\n" // Global variable might be accessed in bar() + " i = 1;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void bar() {}\n" + "void f() {\n" + " int i;\n" + " i = 1;\n" + " bar();\n" + " i = 1;\n" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:6]: (performance) Variable 'i' is reassigned a value before the old one has been used.\n", errout.str()); + + check("void f(int i) {\n" + " i = 1;\n" + " bar();\n" // Unknown function - can be noreturn + " i = 1;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void bar(int i) {}\n" + "void f(int i) {\n" + " i = 1;\n" + " bar(i);\n" // Passed as argument + " i = 1;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // Branch tests + check("void f(int i) {\n" + " i = 1;\n" + " if(x)\n" + " i = 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(int i) {\n" + " if(x)\n" + " i = 0;\n" + " i = 1;\n" + " i = 2;\n" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:5]: (performance) Variable 'i' is reassigned a value before the old one has been used.\n", errout.str()); + } + + void redundantMemWrite() { + // Simple tests + check("void f(void* a) {\n" + " memcpy(a, foo, bar);\n" + " memset(a, 0, bar);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (performance) Buffer 'a' is being written before its old content has been used.\n", errout.str()); + + check("void* a;\n" + "void f() {\n" + " strcpy(a, foo);\n" + " strncpy(a, 0, bar);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (performance) Buffer 'a' is being written before its old content has been used.\n", errout.str()); + + check("void f() {\n" + " void* a = foo();\n" + " sprintf(a, foo);\n" + " memmove(a, 0, bar);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (performance) Buffer 'a' is being written before its old content has been used.\n", errout.str()); + + // Writing to different parts of a buffer + check("void f(void* a) {\n" + " memcpy(a, foo, bar);\n" + " memset(a+5, 0, bar);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // Use variable as second argument + check("void f(void* a, void* b) {\n" + " memset(a, 0, 5);\n" + " memcpy(b, a, 5);\n" + " memset(a, 1, 5);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // strcat is special + check("void f(void* a) {\n" + " strcpy(a, foo);\n" + " strcat(a, bar);\n" // Not redundant + " strcpy(a, x);\n" // Redundant + "}"); + ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:4]: (performance) Buffer 'a' is being written before its old content has been used.\n", errout.str()); + + // Tests with function call between copy + check("void bar() {}\n" + "void f(void* a) {\n" + " snprintf(a, foo, bar);\n" + " bar();\n" + " memset(a, 0, size);\n" + "}"); + ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:5]: (performance) Buffer 'a' is being written before its old content has been used.\n", errout.str()); + + check("void bar() {}\n" + "void* a;\n" + "void f() {\n" + " memset(a, 0, size);\n" + " bar();\n" // Global variable might be accessed in bar() + " memset(a, 0, size);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void bar() {}\n" + "void f() {\n" + " void* a = foo();\n" + " memset(a, 0, size);\n" + " bar();\n" + " memset(a, 0, size);\n" + "}"); + ASSERT_EQUALS("[test.cpp:4] -> [test.cpp:6]: (performance) Buffer 'a' is being written before its old content has been used.\n", errout.str()); + + check("void f(void* a) {\n" + " memset(a, 0, size);\n" + " bar();\n" // Unknown function - can be noreturn + " memset(a, 0, size);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void bar(void* a) {}\n" + "void f(void* a) {\n" + " memset(a, 0, size);\n" + " bar(a);\n" // Passed as argument + " memset(a, 0, size);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + // Branch tests + check("void f(void* a) {\n" + " memset(a, 0, size);\n" + " if(x)\n" + " memset(a, 0, size);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther)