Fixed #2822 (New check: Duplicate break statements in switch)

lib/checkother.cpp

@@ -867,6 +867,35 @@ void CheckOther::checkComparisonOfBoolWithInt()
     }
 }
 
+//---------------------------------------------------------------------------
+//    switch (x)
+//    {
+//        case 2:
+//            y = a;
+//            break;
+//            break; // <- Redundant break
+//        case 3:
+//            y = b;
+//    }
+//---------------------------------------------------------------------------
+void CheckOther::checkDuplicateBreak()
+{
+    if (!_settings->_checkCodingStyle)
+        return;
+
+    const char breakPattern[] = "break|continue ; break|continue ;";
+
+    // 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);
+    }
+}
+
+
 void CheckOther::sizeofForNumericParameterError(const Token *tok)
 {
     reportError(tok, Severity::error,
@@ -3694,3 +3723,10 @@ void CheckOther::comparisonOfBoolWithIntError(const Token *tok, const std::strin
                 "Comparison of a boolean with a non-zero integer\n"
                 "The expression \"!" + varname + "\" is of type 'bool' but is compared against a non-zero 'int'.");
 }
+
+void CheckOther::duplicateBreakError(const Token *tok)
+{
+    reportError(tok, Severity::style, "duplicateBreak",
+                "Consecutive break or continue statements are unnecessary\n"
+                "The second of the two statements can never be executed, and so should be removed\n");
+}

lib/checkother.h

@@ -67,6 +67,7 @@ public:
         checkOther.checkDuplicateIf();
         checkOther.checkDuplicateBranch();
         checkOther.checkDuplicateExpression();
+        checkOther.checkDuplicateBreak();
 
         // information checks
         checkOther.checkVariableScope();
@@ -224,9 +225,13 @@ 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 if token is a record type without side effects */
     bool isRecordTypeWithoutSideEffects(const Token *tok);
 
+
     // Error messages..
     void cstyleCastError(const Token *tok);
     void dangerousUsageStrtolError(const Token *tok);
@@ -260,6 +265,7 @@ public:
     void duplicateBranchError(const Token *tok1, const Token *tok2);
     void duplicateExpressionError(const Token *tok1, const Token *tok2, const std::string &op);
     void alwaysTrueFalseStringCompare(const Token *tok, const std::string& str1, const std::string& str2);
+    void duplicateBreakError(const Token *tok);
 
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings)
     {
@@ -309,6 +315,7 @@ public:
         c.duplicateBranchError(0, 0);
         c.duplicateExpressionError(0, 0, "&&");
         c.alwaysTrueFalseStringCompare(0, "str1", "str2");
+        c.duplicateBreakError(0);
     }
 
     std::string myName() const
@@ -354,6 +361,7 @@ public:
                "* comparison of a boolean with a non-zero integer\n"
                "* suspicious condition (assignment+comparison)\n"
                "* suspicious condition (runtime comparison of string literals)\n"
+               "* duplicate break statement\n"
 
                // optimisations
                "* optimisation: detect post increment/decrement\n";

test/testother.cpp

@@ -77,6 +77,7 @@ private:
 
         TEST_CASE(switchRedundantAssignmentTest);
         TEST_CASE(switchFallThroughCase);
+        TEST_CASE(duplicateBreak);
 
         TEST_CASE(selfAssignment);
         TEST_CASE(testScanf1);
@@ -166,6 +167,7 @@ private:
         checkOther.checkIncorrectStringCompare();
         checkOther.checkIncrementBoolean();
         checkOther.checkComparisonOfBoolWithInt();
+        checkOther.checkDuplicateBreak();
     }
 
     class SimpleSuppressor: public ErrorLogger
@@ -1558,6 +1560,71 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void duplicateBreak()
+    {
+        check("void foo(int a)\n"
+              "{\n"
+              "        switch(a) {\n"
+              "          case 0:\n"
+              "            printf(\"case 0\");\n"
+              "            break;\n"
+              "            break;\n"
+              "          case 1:\n"
+              "            c++;\n"
+              "            break;\n"
+              "         }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:6]: (style) Consecutive break or continue statements are unnecessary\n", errout.str());
+
+        check("void foo(int a)\n"
+              "{\n"
+              "        switch(a) {\n"
+              "          case 0:\n"
+              "            printf(\"case 0\");\n"
+              "            break;\n"
+              "          case 1:\n"
+              "            c++;\n"
+              "            break;\n"
+              "         }\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void foo(int a)\n"
+              "{\n"
+              "        while(1) {\n"
+              "          if (a++ >= 100) {\n"
+              "            break;\n"
+              "            break;\n"
+              "          }\n"
+              "       }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (style) Consecutive break or continue statements are unnecessary\n", errout.str());
+
+        check("void foo(int a)\n"
+              "{\n"
+              "        while(1) {\n"
+              "          if (a++ >= 100) {\n"
+              "            continue;\n"
+              "            continue;\n"
+              "          }\n"
+              "          a+=2;\n"
+              "       }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (style) Consecutive break or continue statements are unnecessary\n", errout.str());
+
+        check("void foo(int a)\n"
+              "{\n"
+              "        while(1) {\n"
+              "          if (a++ >= 100) {\n"
+              "            continue;\n"
+              "          }\n"
+              "          a+=2;\n"
+              "       }\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
+
+
     void selfAssignment()
     {
         check("void foo()\n"