diff --git a/lib/checkother.cpp b/lib/checkother.cpp index c96478394..9bdb47c88 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -267,6 +267,89 @@ void CheckOther::checkFflushOnInputStream() } } +//--------------------------------------------------------------------------- +// switch (x) +// { +// case 2: +// y = a; // <- this assignment is redundant +// case 3: +// y = b; // <- case 2 falls through and sets y twice +// } +//--------------------------------------------------------------------------- +void CheckOther::checkRedundantAssignmentInSwitch() +{ + const char switchPattern[] = "switch ( %any% ) { case"; + const char breakPattern[] = "break|return|exit|goto"; + const char functionPattern[] = "%var% ("; + + // Find the beginning of a switch. E.g.: + // switch (var) { ... + const Token *tok = Token::findmatch(_tokenizer->tokens(), switchPattern); + while (tok) + { + + // Check the contents of the switch statement + std::map varsAssigned; + int indentLevel = 0; + for (const Token *tok2 = tok->tokAt(5); tok2; tok2 = tok2->next()) + { + if (tok2->str() == "{") + { + // Inside a conditional or loop. Don't mark variable accesses as being redundant. E.g.: + // case 3: b = 1; + // case 4: if (a) { b = 2; } // Doesn't make the b=1 redundant because it's conditional + if (Token::Match(tok2->previous(), ")|else {") && tok2->link()) + { + const Token* endOfConditional = tok2->link(); + for (const Token* tok3 = tok2; tok3 != endOfConditional; tok3 = tok3->next()) + { + if (tok3->varId() != 0) + varsAssigned.erase(tok3->varId()); + else if (Token::Match(tok3, functionPattern) || Token::Match(tok3, breakPattern)) + varsAssigned.clear(); + } + tok2 = endOfConditional; + } + else + ++ indentLevel; + } + else if (tok2->str() == "}") + { + -- indentLevel; + + // End of the switch block + if (indentLevel < 0) + break; + } + + // Variable assignment. Report an error if it's assigned to twice before a break. E.g.: + // case 3: b = 1; // <== redundant + // case 4: b = 2; + if (Token::Match(tok2->previous(), ";|{|}|: %var% = %any% ;") && tok2->varId() != 0) + { + std::map::iterator i = varsAssigned.find(tok2->varId()); + if (i == varsAssigned.end()) + varsAssigned[tok2->varId()] = tok2; + else + redundantAssignmentInSwitchError(i->second, i->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) + varsAssigned.erase(tok2->varId()); + + // 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(); + + } + + tok = Token::findmatch(tok->next(), switchPattern); + } +} + //--------------------------------------------------------------------------- // strtol(str, 0, radix) <- radix must be 0 or 2-36 //--------------------------------------------------------------------------- @@ -3773,3 +3856,9 @@ void CheckOther::sizeofsizeofError(const Token *tok) reportError(tok, Severity::style, "sizeofsizeof", "Suspicious code 'sizeof sizeof ..', most likely there should only be one sizeof. The current code is equivalent to 'sizeof(size_t)'."); } + +void CheckOther::redundantAssignmentInSwitchError(const Token *tok, const std::string &varname) +{ + reportError(tok, Severity::style, + "redundantAssignInSwitch", "Redundant assignment of \"" + varname + "\" in switch"); +} diff --git a/lib/checkother.h b/lib/checkother.h index 784307e70..efac7d600 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -62,6 +62,7 @@ public: checkOther.strPlusChar(); checkOther.sizeofsizeof(); checkOther.checkEmptyCatchBlock(); + checkOther.checkRedundantAssignmentInSwitch(); } /** @brief Run checks against the simplified token list */ @@ -178,6 +179,9 @@ public: void sizeofsizeof(); void sizeofsizeofError(const Token *tok); + /** @brief %Check for assigning to the same variable twice in a switch statement*/ + void checkRedundantAssignmentInSwitch(); + // Error messages.. void cstyleCastError(const Token *tok); void redundantIfDelete0Error(const Token *tok); @@ -205,6 +209,7 @@ public: void emptyStringTestError(const Token *tok, const std::string &var_name, const bool isTestForEmpty); void fflushOnInputStreamError(const Token *tok, const std::string &varname); void emptyCatchBlockError(const Token *tok); + void redundantAssignmentInSwitchError(const Token *tok, const std::string &varname); void getErrorMessages() { @@ -234,6 +239,7 @@ public: strPlusChar(0); sizeofsizeofError(0); emptyCatchBlockError(0); + redundantAssignmentInSwitchError(0, "varname"); // optimisations postIncrementError(0, "varname", true); @@ -269,6 +275,7 @@ public: "* condition that is always true/false\n" "* unusal pointer arithmetic. For example: \"abc\" + 'd'\n" "* empty catch() block\n" + "* redundant assignment in a switch statement\n" // optimisations "* optimisation: detect post increment/decrement\n" diff --git a/test/testother.cpp b/test/testother.cpp index dd62163c0..226029682 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -100,6 +100,8 @@ private: TEST_CASE(sizeofsizeof); TEST_CASE(emptyCatchBlock); + + TEST_CASE(switchRedundantAssignmentTest); } void check(const char code[]) @@ -119,6 +121,7 @@ private: checkOther.sizeofsizeof(); checkOther.checkEmptyCatchBlock(); + checkOther.checkRedundantAssignmentInSwitch(); // Simplify token list.. tokenizer.simplifyTokenList(); @@ -2715,6 +2718,141 @@ private: "}\n"); ASSERT_EQUALS("", errout.str()); } + + void switchRedundantAssignmentTest() + { + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " y = 2;\n" + " case 3:\n" + " y = 3;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:7]: (style) Redundant assignment of \"y\" in switch\n", errout.str()); + + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " {\n" + " y = 2;\n" + " }\n" + " case 3:\n" + " y = 3;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:8]: (style) Redundant assignment of \"y\" in switch\n", errout.str()); + + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " y = 2;\n" + " case 3:\n" + " if (x)\n" + " {\n" + " y = 3;\n" + " }\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " {\n" + " y = 2;\n" + " if (y)\n" + " printf(\"%d\", y);\n" + " }\n" + " case 3:\n" + " y = 3;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void foo()\n" + "{\n" + " int x = a;\n" + " int y = 1;\n" + " switch (x)\n" + " {\n" + " case 2:\n" + " x = 2;\n" + " case 3:\n" + " y = 3;\n" + " }\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 = 2;\n" + " }\n" + " case 3:\n" + " y = 3;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (x)\n" + " {\n" + " case 2:\n" + " y = 2;\n" + " break;\n" + " case 3:\n" + " y = 3;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (x)\n" + " {\n" + " case 2:\n" + " y = 2;\n" + " printf(\"%d\", y);\n" + " case 3:\n" + " y = 3;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (x)\n" + " {\n" + " case 2:\n" + " y = 2;\n" + " bar();\n" + " case 3:\n" + " y = 3;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestOther)