diff --git a/lib/checkother.cpp b/lib/checkother.cpp index d50cfd6bf..8dc4c1db1 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1439,39 +1439,75 @@ void CheckOther::comparisonOfBoolWithIntError(const Token *tok, const std::strin } //--------------------------------------------------------------------------- -// switch (x) -// { -// case 2: -// y = a; -// break; -// break; // <- Redundant break -// case 3: -// y = b; -// } +// Find consecutive return, break, continue, goto or throw statements. e.g.: +// break; break; +// Detect dead code, that follows such a statement. e.g.: +// return(0); foo(); //--------------------------------------------------------------------------- -void CheckOther::checkDuplicateBreak() +void CheckOther::checkUnreachableCode() { if (!_settings->isEnabled("style")) return; - const char breakPattern[] = "break|continue ; break|continue ;"; + for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { + const Token* secondBreak = 0; + if (Token::Match(tok, "break|continue ;")) + secondBreak = tok->tokAt(2); + else if (tok->str() == "return" || tok->str() == "throw") { + for (const Token *tok2 = tok->next(); tok2; tok2 = tok2->next()) + if (tok2->str() == ";") { + secondBreak = tok2->tokAt(1); + break; + } + } else if (Token::Match(tok, "goto %any% ;")) + secondBreak = tok->tokAt(3); - // Find consecutive break or continue statements. e.g.: - // break; break; - const Token *tok = Token::findmatch(_tokenizer->tokens(), breakPattern); - while (tok) { - duplicateBreakError(tok); - tok = Token::findmatch(tok->next(), breakPattern); + if (secondBreak) { + if (Token::Match(secondBreak, "continue|goto|throw") || + (secondBreak->str() == "return" && (tok->str() == "return" || secondBreak->strAt(1) == ";"))) { // return with value after statements like throw can be necessary to make a function compile + duplicateBreakError(secondBreak); + tok = Token::findmatch(secondBreak, "[}:]"); + } else if (secondBreak->str() == "break") { // break inside switch as second break statement should not issue a warning + if (tok->str() == "break") // If the previous was a break, too: Issue warning + duplicateBreakError(secondBreak); + else { + unsigned int indent = 0; + for (const Token* tok2 = tok; tok2; tok2 = tok2->previous()) { // Check, if the enclosing scope is a switch (TODO: Can we use SymbolDatabase here?) + if (tok2->str() == "}") + indent++; + else if (indent == 0 && tok2->str() == "{" && tok2->strAt(-1) == ")") { + if (tok2->previous()->link()->strAt(-1) != "switch") { + duplicateBreakError(secondBreak); + break; + } else + break; + } else if (tok2->str() == "{") + indent--; + } + } + tok = Token::findmatch(secondBreak, "[}:]"); + } else if (!Token::Match(secondBreak, "return|}|case|default") && secondBreak->strAt(1) != ":") { // TODO: No bailout for unconditional scopes + unreachableCodeError(secondBreak); + tok = Token::findmatch(secondBreak, "[}:]"); + } else + tok = secondBreak; + } } } void CheckOther::duplicateBreakError(const Token *tok) { reportError(tok, Severity::style, "duplicateBreak", - "Consecutive break or continue statements are unnecessary\n" + "Consecutive return, break, continue, goto or throw statements are unnecessary\n" "The second of the two statements can never be executed, and so should be removed\n"); } +void CheckOther::unreachableCodeError(const Token *tok) +{ + reportError(tok, Severity::style, "unreachableCode", + "Statements following return, break, continue, goto or throw will never be executed\n"); +} + //--------------------------------------------------------------------------- // Check for unsigned divisions //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index b997d87c3..9f6532160 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -65,7 +65,7 @@ public: checkOther.checkDuplicateIf(); checkOther.checkDuplicateBranch(); checkOther.checkDuplicateExpression(); - checkOther.checkDuplicateBreak(); + checkOther.checkUnreachableCode(); checkOther.checkSuspiciousSemicolon(); // information checks @@ -237,8 +237,8 @@ public: /** @brief %Check for suspicious code that compares string literals for equality */ void checkAlwaysTrueOrFalseStringCompare(); - /** @brief %Check for duplicate break statements in a switch or loop */ - void checkDuplicateBreak(); + /** @brief %Check for code that gets never executed, such as duplicate break statements */ + void checkUnreachableCode(); /** @brief assigning bool to pointer */ void checkAssignBoolToPointer(); @@ -294,6 +294,7 @@ public: void alwaysTrueFalseStringCompareError(const Token *tok, const std::string& str1, const std::string& str2); void alwaysTrueStringVariableCompareError(const Token *tok, const std::string& str1, const std::string& str2); void duplicateBreakError(const Token *tok); + void unreachableCodeError(const Token* tok); void assignBoolToPointerError(const Token *tok); void unsignedLessThanZeroError(const Token *tok, const std::string &varname, bool inconclusive); void unsignedPositiveError(const Token *tok, const std::string &varname, bool inconclusive); @@ -349,6 +350,7 @@ public: c.alwaysTrueFalseStringCompareError(0, "str1", "str2"); c.alwaysTrueStringVariableCompareError(0, "varname1", "varname2"); c.duplicateBreakError(0); + c.unreachableCodeError(0); c.unsignedLessThanZeroError(0, "varname", false); c.unsignedPositiveError(0, "varname", false); c.bitwiseOnBooleanError(0, "varname", "&&"); @@ -405,6 +407,7 @@ public: "* suspicious condition (runtime comparison of string literals)\n" "* suspicious condition (string literals as boolean)\n" "* duplicate break statement\n" + "* unreachable code\n" "* testing if unsigned variable is negative\n" "* testing is unsigned variable is positive\n" "* using bool in bitwise expression\n" diff --git a/test/testother.cpp b/test/testother.cpp index 431131308..520bb4fcb 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -1765,6 +1765,29 @@ private: } void duplicateBreak() { + check("void foo(int a) {\n" + " while(1) {\n" + " if (a++ >= 100) {\n" + " break;\n" + " continue;\n" + " }\n" + " }\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary\n", errout.str()); + + check("int foo(int a) {\n" + " return 0;\n" + " return(a-1);\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary\n", errout.str()); + + check("int foo(int a) {\n" + " A:" + " return(0);\n" + " goto A;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:3]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary\n", errout.str()); + check("void foo(int a)\n" "{\n" " switch(a) {\n" @@ -1777,7 +1800,7 @@ private: " break;\n" " }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:6]: (style) Consecutive break or continue statements are unnecessary\n", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary\n", errout.str()); check("void foo(int a)\n" "{\n" @@ -1801,7 +1824,7 @@ private: " }\n" " }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:5]: (style) Consecutive break or continue statements are unnecessary\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary\n", errout.str()); check("void foo(int a)\n" "{\n" @@ -1813,7 +1836,7 @@ private: " a+=2;\n" " }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:5]: (style) Consecutive break or continue statements are unnecessary\n", errout.str()); + ASSERT_EQUALS("[test.cpp:6]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary\n", errout.str()); check("void foo(int a)\n" "{\n" @@ -1825,6 +1848,63 @@ private: " }\n" "}\n"); ASSERT_EQUALS("", errout.str()); + + check("int foo() {\n" + " throw 0;\n" + " return 1;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo() {\n" + " throw 0;\n" + " return;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary\n", errout.str()); + + check("int foo() {\n" + " return 0;\n" + " return 1;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary\n", errout.str()); + + check("int foo() {\n" + " return 0;\n" + " foo();\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (style) Statements following return, break, continue, goto or throw will never be executed\n", errout.str()); + + check("int foo() {\n" + " if(bar)\n" + " return 0;\n" + " return 124;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("int foo() {\n" + " while(bar) {\n" + " return 0;\n" + " return 0;\n" + " return 0;\n" + " return 0;\n" + " }\n" + " return 124;\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary\n", errout.str()); + + check("void foo() {\n" + " while(bar) {\n" + " return;\n" + " break;\n" + " }\n" + "}"); + ASSERT_EQUALS("[test.cpp:4]: (style) Consecutive return, break, continue, goto or throw statements are unnecessary\n", errout.str()); + + check("int foo() {\n" + " return 0;\n" + " label:\n" + " throw 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); }