Fixed #3375 (Improve check: Detect unreachable code)

lib/checkother.cpp

@@ -1439,39 +1439,75 @@ void CheckOther::comparisonOfBoolWithIntError(const Token *tok, const std::strin
 }
 
 //---------------------------------------------------------------------------
-//    switch (x)
+//    Find consecutive return, break, continue, goto or throw statements. e.g.:
-//    {
+//        break; break;
-//        case 2:
+//    Detect dead code, that follows such a statement. e.g.:
-//            y = a;
+//        return(0); foo();
-//            break;
-//            break; // <- Redundant break
-//        case 3:
-//            y = b;
-//    }
 //---------------------------------------------------------------------------
-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.:
+        if (secondBreak) {
-    //   break; break;
+            if (Token::Match(secondBreak, "continue|goto|throw") ||
-    const Token *tok = Token::findmatch(_tokenizer->tokens(), breakPattern);
+                (secondBreak->str() == "return" && (tok->str() == "return" || secondBreak->strAt(1) == ";"))) { // return with value after statements like throw can be necessary to make a function compile
-    while (tok) {
+                duplicateBreakError(secondBreak);
-        duplicateBreakError(tok);
+                tok = Token::findmatch(secondBreak, "[}:]");
-        tok = Token::findmatch(tok->next(), breakPattern);
+            } 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
 //---------------------------------------------------------------------------

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 */
+    /** @brief %Check for code that gets never executed, such as duplicate break statements */
-    void checkDuplicateBreak();
+    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"

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());
     }