From afe030ce9bf6ab35f16903c9138209b56774b581 Mon Sep 17 00:00:00 2001 From: Kumar Ashwani Date: Wed, 22 Aug 2012 14:40:57 +0200 Subject: [PATCH] Fixed #2628: Detect redudant usage of operator++/-- in switch. --- lib/checkother.cpp | 41 +++++- lib/checkother.h | 3 + test/testother.cpp | 356 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 396 insertions(+), 4 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 4581d6937..5c5762c87 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -605,6 +605,14 @@ void CheckOther::checkRedundantAssignmentInSwitch() 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() == "{") { @@ -616,6 +624,7 @@ void CheckOther::checkRedundantAssignmentInSwitch() 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()); @@ -623,6 +632,7 @@ void CheckOther::checkRedundantAssignmentInSwitch() varsAssigned.clear(); varsWithBitsSet.clear(); bitOperations.clear(); + varsOperatedByPostORPreFix.erase(tok3->varId()); if (tok3->str() != "strcpy" && tok3->str() != "strncpy") stringsCopied.clear(); @@ -635,18 +645,33 @@ void CheckOther::checkRedundantAssignmentInSwitch() // 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 i2 = varsAssigned.find(tok2->varId()); - if (i2 == varsAssigned.end()) - varsAssigned[tok2->varId()] = tok2; - else - redundantAssignmentInSwitchError(i2->second, i2->second->str()); + 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; @@ -700,6 +725,7 @@ void CheckOther::checkRedundantAssignmentInSwitch() varsAssigned.clear(); varsWithBitsSet.clear(); bitOperations.clear(); + varsOperatedByPostORPreFix.clear(); if (tok2->str() != "strcpy" && tok2->str() != "strncpy") stringsCopied.clear(); @@ -714,6 +740,13 @@ void CheckOther::redundantAssignmentInSwitchError(const Token *tok, const std::s "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, diff --git a/lib/checkother.h b/lib/checkother.h index fc0354ebd..01a87836c 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -261,6 +261,7 @@ private: 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 redundantBitwiseOperationInSwitchError(const Token *tok, const std::string &varname); void redundantStrcpyInSwitchError(const Token *tok, const std::string &varname); void switchCaseFallThrough(const Token *tok); @@ -329,6 +330,7 @@ private: c.sizeofsizeofError(0); c.sizeofCalculationError(0, false); c.redundantAssignmentInSwitchError(0, "varname"); + c.redundantOperationInSwitchError(0, "varname"); c.switchCaseFallThrough(0); c.selfAssignmentError(0, "varname"); c.assignmentInAssertError(0, "varname"); @@ -393,6 +395,7 @@ private: "* condition that is always true/false\n" "* unusal pointer arithmetic. For example: \"abc\" + 'd'\n" "* redundant assignment in a switch statement\n" + "* redundant pre/post operation in a switch statement\n" "* redundant bitwise operation in a switch statement\n" "* redundant strcpy in a switch statement\n" "* look for 'sizeof sizeof ..'\n" diff --git a/test/testother.cpp b/test/testother.cpp index d43596983..abf806838 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -80,6 +80,7 @@ private: TEST_CASE(sizeofCalculation); TEST_CASE(switchRedundantAssignmentTest); + TEST_CASE(switchRedundantOperationTest); TEST_CASE(switchRedundantBitwiseOperationTest); TEST_CASE(switchFallThroughCase); TEST_CASE(unreachableCode); @@ -1606,6 +1607,361 @@ private: ASSERT_EQUALS("", errout.str()); } + void switchRedundantOperationTest() { + + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " ++y;\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:7]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " {\n" + " ++y;\n" + " }\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:8]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " y;\n" + " case 3:\n" + " ++y;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " ++y;\n" + " case 3:\n" + " ++y;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " --y;\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:7]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " {\n" + " --y;\n" + " }\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:8]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " y;\n" + " case 3:\n" + " --y;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " --y;\n" + " case 3:\n" + " --y;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " y++;\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:7]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " {\n" + " y++;\n" + " }\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:8]: (warning) Redundant operation on '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" + " y++;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " y++;\n" + " case 3:\n" + " y++;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " y--;\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:7]: (warning) Redundant operation on 'y' in switch.\n", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " {\n" + " y--;\n" + " }\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:8]: (warning) Redundant operation on '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" + " y--;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " y--;\n" + " case 3:\n" + " y--;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " y++;\n" + " case 3:\n" + " if (x)\n" + " {\n" + " y = 3;\n" + " }\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (a)\n" + " {\n" + " case 2:\n" + " {\n" + " y++;\n" + " if (y)\n" + " printf(\"%d\", y);\n" + " }\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\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++;\n" + " case 3:\n" + " y++;\n" + " }\n" + " 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" + " switch (x)\n" + " {\n" + " case 2:\n" + " y++;\n" + " break;\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" + " while(xyz()) {\n" + " switch (x)\n" + " {\n" + " case 2:\n" + " y++;\n" + " continue;\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " while(xyz()) {\n" + " switch (x)\n" + " {\n" + " case 2:\n" + " y++;\n" + " throw e;\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + check("void foo()\n" + "{\n" + " int y = 1;\n" + " switch (x)\n" + " {\n" + " case 2:\n" + " y++;\n" + " printf(\"%d\", y);\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" + " switch (x)\n" + " {\n" + " case 2:\n" + " y++;\n" + " bar();\n" + " case 3:\n" + " y = 3;\n" + " }\n" + " bar(y);\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } void switchRedundantBitwiseOperationTest() { check("void foo(int a)\n"