Merge branch 'switch-case-fall-through'

lib/checkother.cpp

@@ -24,6 +24,7 @@
 
 #include <cctype> // std::isupper
 #include <cmath> // fabs()
+#include <stack>
 //---------------------------------------------------------------------------
 
 // Register this check class (by creating a static instance of it)
@@ -306,6 +307,173 @@ void CheckOther::checkRedundantAssignmentInSwitch()
 }
 
 
+void CheckOther::checkSwitchCaseFallThrough()
+{
+    const char switchPattern[] = "switch (";
+    const char breakPattern[] = "break|continue|return|exit|goto";
+
+    // 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::stack<std::pair<Token *, bool> > ifnest;
+        std::stack<Token *> loopnest;
+        std::stack<Token *> scopenest;
+        bool justbreak = true;
+        bool firstcase = true;
+        for (const Token *tok2 = tok->tokAt(1)->link()->tokAt(2); tok2; tok2 = tok2->next())
+        {
+            if (Token::Match(tok2, "if ("))
+            {
+                tok2 = tok2->tokAt(1)->link()->next();
+                if (tok2->link() == NULL)
+                {
+                    std::ostringstream errmsg;
+                    errmsg << "unmatched if in switch: " << tok2->linenr();
+                    reportError(_tokenizer->tokens(), Severity::debug, "debug", errmsg.str());
+                    break;
+                }
+                ifnest.push(std::make_pair(tok2->link(), false));
+                justbreak = false;
+            }
+            else if (Token::Match(tok2, "while ("))
+            {
+                tok2 = tok2->tokAt(1)->link()->next();
+                // skip over "do { } while ( ) ;" case
+                if (tok2->str() == "{")
+                {
+                    if (tok2->link() == NULL)
+                    {
+                        std::ostringstream errmsg;
+                        errmsg << "unmatched while in switch: " << tok2->linenr();
+                        reportError(_tokenizer->tokens(), Severity::debug, "debug", errmsg.str());
+                        break;
+                    }
+                    loopnest.push(tok2->link());
+                }
+                justbreak = false;
+            }
+            else if (Token::Match(tok2, "do {"))
+            {
+                tok2 = tok2->tokAt(1);
+                if (tok2->link() == NULL)
+                {
+                    std::ostringstream errmsg;
+                    errmsg << "unmatched do in switch: " << tok2->linenr();
+                    reportError(_tokenizer->tokens(), Severity::debug, "debug", errmsg.str());
+                    break;
+                }
+                loopnest.push(tok2->link());
+                justbreak = false;
+            }
+            else if (Token::Match(tok2, "for ("))
+            {
+                tok2 = tok2->tokAt(1)->link()->next();
+                if (tok2->link() == NULL)
+                {
+                    std::ostringstream errmsg;
+                    errmsg << "unmatched for in switch: " << tok2->linenr();
+                    reportError(_tokenizer->tokens(), Severity::debug, "debug", errmsg.str());
+                    break;
+                }
+                loopnest.push(tok2->link());
+                justbreak = false;
+            }
+            else if (Token::Match(tok2, switchPattern))
+            {
+                // skip over nested switch, we'll come to that soon
+                tok2 = tok2->tokAt(1)->link()->next()->link();
+            }
+            else if (Token::Match(tok2, breakPattern))
+            {
+                if (loopnest.empty())
+                {
+                    justbreak = true;
+                }
+                tok2 = Token::findmatch(tok2, ";");
+            }
+            else if (Token::Match(tok2, "case|default"))
+            {
+                if (!justbreak && !firstcase)
+                {
+                    switchCaseFallThrough(tok2);
+                }
+                tok2 = Token::findmatch(tok2, ":");
+                justbreak = true;
+                firstcase = false;
+            }
+            else if (tok2->str() == "{")
+            {
+                scopenest.push(tok2->link());
+            }
+            else if (tok2->str() == "}")
+            {
+                if (!ifnest.empty() && tok2 == ifnest.top().first)
+                {
+                    if (tok2->next()->str() == "else")
+                    {
+                        tok2 = tok2->tokAt(2);
+                        ifnest.pop();
+                        if (tok2->link() == NULL)
+                        {
+                            std::ostringstream errmsg;
+                            errmsg << "unmatched if in switch: " << tok2->linenr();
+                            reportError(_tokenizer->tokens(), Severity::debug, "debug", errmsg.str());
+                            break;
+                        }
+                        ifnest.push(std::make_pair(tok2->link(), justbreak));
+                        justbreak = false;
+                    }
+                    else
+                    {
+                        justbreak &= ifnest.top().second;
+                        ifnest.pop();
+                    }
+                }
+                else if (!loopnest.empty() && tok2 == loopnest.top())
+                {
+                    loopnest.pop();
+                }
+                else if (!scopenest.empty() && tok2 == scopenest.top())
+                {
+                    scopenest.pop();
+                }
+                else
+                {
+                    if (!ifnest.empty() || !loopnest.empty() || !scopenest.empty())
+                    {
+                        std::ostringstream errmsg;
+                        errmsg << "unexpected end of switch: ";
+                        errmsg << "ifnest=" << ifnest.size();
+                        if (!ifnest.empty())
+                            errmsg << "," << ifnest.top().first->linenr();
+                        errmsg << ", loopnest=" << loopnest.size();
+                        if (!loopnest.empty())
+                            errmsg << "," << loopnest.top()->linenr();
+                        errmsg << ", scopenest=" << scopenest.size();
+                        if (!scopenest.empty())
+                            errmsg << "," << scopenest.top()->linenr();
+                        reportError(_tokenizer->tokens(), Severity::debug, "debug", errmsg.str());
+                    }
+                    // end of switch block
+                    break;
+                }
+            }
+            else if (tok2->str() != ";")
+            {
+                justbreak = false;
+            }
+
+        }
+
+        tok = Token::findmatch(tok->next(), switchPattern);
+    }
+}
+
+
 //---------------------------------------------------------------------------
 //    int x = 1;
 //    x = x;            // <- redundant assignment to self
@@ -2998,6 +3166,12 @@ void CheckOther::redundantAssignmentInSwitchError(const Token *tok, const std::s
                 "redundantAssignInSwitch", "Redundant assignment of \"" + varname + "\" in switch");
 }
 
+void CheckOther::switchCaseFallThrough(const Token *tok)
+{
+    reportError(tok, Severity::warning,
+                "switchCaseFallThrough", "Switch falls through case without comment");
+}
+
 void CheckOther::selfAssignmentError(const Token *tok, const std::string &varname)
 {
     reportError(tok, Severity::warning,

lib/checkother.h

@@ -90,6 +90,7 @@ public:
         checkOther.checkIncorrectStringCompare();
         checkOther.checkIncrementBoolean();
         checkOther.checkComparisonOfBoolWithInt();
+        checkOther.checkSwitchCaseFallThrough();
     }
 
     /** @brief Clarify calculation for ".. a * b ? .." */
@@ -162,6 +163,9 @@ public:
     /** @brief %Check for assigning to the same variable twice in a switch statement*/
     void checkRedundantAssignmentInSwitch();
 
+    /** @brief %Check for switch case fall through without comment */
+    void checkSwitchCaseFallThrough();
+
     /** @brief %Check for assigning a variable to itself*/
     void checkSelfAssignment();
 
@@ -209,6 +213,7 @@ public:
     void mathfunctionCallError(const Token *tok, const unsigned int numParam = 1);
     void fflushOnInputStreamError(const Token *tok, const std::string &varname);
     void redundantAssignmentInSwitchError(const Token *tok, const std::string &varname);
+    void switchCaseFallThrough(const Token *tok);
     void selfAssignmentError(const Token *tok, const std::string &varname);
     void assignmentInAssertError(const Token *tok, const std::string &varname);
     void incorrectLogicOperatorError(const Token *tok);
@@ -247,6 +252,7 @@ public:
         c.sizeofsizeofError(0);
         c.sizeofCalculationError(0);
         c.redundantAssignmentInSwitchError(0, "varname");
+        c.switchCaseFallThrough(0);
         c.selfAssignmentError(0, "varname");
         c.assignmentInAssertError(0, "varname");
         c.invalidScanfError(0);

lib/preprocessor.cpp

@@ -303,6 +303,18 @@ static bool hasbom(const std::string &str)
 }
 
 
+static bool isFallThroughComment(std::string comment)
+{
+    // convert comment to lower case without whitespace
+    std::transform(comment.begin(), comment.end(), comment.begin(), ::tolower);
+    comment.erase(std::remove_if(comment.begin(), comment.end(), ::isspace), comment.end());
+
+    return comment.find("fallthr") != std::string::npos ||
+           comment.find("dropthr") != std::string::npos ||
+           comment.find("passthr") != std::string::npos ||
+           comment.find("nobreak") != std::string::npos;
+}
+
 std::string Preprocessor::removeComments(const std::string &str, const std::string &filename, Settings *settings)
 {
     // For the error report
@@ -314,6 +326,7 @@ std::string Preprocessor::removeComments(const std::string &str, const std::stri
     unsigned int newlines = 0;
     std::ostringstream code;
     unsigned char previous = 0;
+    bool inPreprocessorLine = false;
     std::vector<std::string> suppressionIDs;
 
     for (std::string::size_type i = hasbom(str) ? 3U : 0U; i < str.length(); ++i)
@@ -358,6 +371,8 @@ std::string Preprocessor::removeComments(const std::string &str, const std::stri
             // if there has been <backspace><newline> sequences, add extra newlines..
             if (ch == '\n')
             {
+                if (previous != '\\')
+                    inPreprocessorLine = false;
                 ++lineno;
                 if (newlines > 0)
                 {
@@ -377,10 +392,10 @@ std::string Preprocessor::removeComments(const std::string &str, const std::stri
             i = str.find('\n', i);
             if (i == std::string::npos)
                 break;
+            std::string comment(str, commentStart, i - commentStart);
 
             if (settings && settings->_inlineSuppressions)
             {
-                std::string comment(str, commentStart, i - commentStart);
                 std::istringstream iss(comment);
                 std::string word;
                 iss >> word;
@@ -392,6 +407,11 @@ std::string Preprocessor::removeComments(const std::string &str, const std::stri
                 }
             }
 
+            if (isFallThroughComment(comment))
+            {
+                suppressionIDs.push_back("switchCaseFallThrough");
+            }
+
             code << "\n";
             previous = '\n';
             ++lineno;
@@ -412,10 +432,15 @@ std::string Preprocessor::removeComments(const std::string &str, const std::stri
                     ++lineno;
                 }
             }
+            std::string comment(str, commentStart, i - commentStart);
+
+            if (isFallThroughComment(comment))
+            {
+                suppressionIDs.push_back("switchCaseFallThrough");
+            }
 
             if (settings && settings->_inlineSuppressions)
             {
-                std::string comment(str, commentStart, i - commentStart);
                 std::istringstream iss(comment);
                 std::string word;
                 iss >> word;
@@ -427,25 +452,35 @@ std::string Preprocessor::removeComments(const std::string &str, const std::stri
                 }
             }
         }
+        else if (ch == '#' && previous == '\n')
+        {
+            code << ch;
+            previous = ch;
+            inPreprocessorLine = true;
+        }
         else
         {
-            // Not whitespace and not a comment. Must be code here!
+            if (!inPreprocessorLine)
-            // Add any pending inline suppressions that have accumulated.
-            if (!suppressionIDs.empty())
             {
-                if (settings != NULL)
+                // Not whitespace, not a comment, and not preprocessor.
+                // Must be code here!
+                // Add any pending inline suppressions that have accumulated.
+                if (!suppressionIDs.empty())
                 {
-                    // Add the suppressions.
+                    if (settings != NULL)
-                    for (size_t j(0); j < suppressionIDs.size(); ++j)
                     {
-                        const std::string errmsg(settings->nomsg.addSuppression(suppressionIDs[j], filename, lineno));
+                        // Add the suppressions.
-                        if (!errmsg.empty())
+                        for (size_t j(0); j < suppressionIDs.size(); ++j)
                         {
-                            writeError(filename, lineno, _errorLogger, "cppcheckError", errmsg);
+                            const std::string errmsg(settings->nomsg.addSuppression(suppressionIDs[j], filename, lineno));
+                            if (!errmsg.empty())
+                            {
+                                writeError(filename, lineno, _errorLogger, "cppcheckError", errmsg);
+                            }
                         }
                     }
+                    suppressionIDs.clear();
                 }
-                suppressionIDs.clear();
             }
 
             // String or char constants..

test/testother.cpp

@@ -16,6 +16,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include "preprocessor.h"
 #include "tokenize.h"
 #include "checkother.h"
 #include "testsuite.h"
@@ -74,6 +75,7 @@ private:
         TEST_CASE(sizeofCalculation);
 
         TEST_CASE(switchRedundantAssignmentTest);
+        TEST_CASE(switchFallThroughCase);
 
         TEST_CASE(selfAssignment);
         TEST_CASE(testScanf1);
@@ -148,6 +150,63 @@ private:
         checkOther.checkComparisonOfBoolWithInt();
     }
 
+    class SimpleSuppressor: public ErrorLogger
+    {
+    public:
+        SimpleSuppressor(Settings &settings, ErrorLogger *next)
+            : _settings(settings), _next(next)
+        { }
+        virtual void reportOut(const std::string &outmsg)
+        {
+            _next->reportOut(outmsg);
+        }
+        virtual void reportErr(const ErrorLogger::ErrorMessage &msg)
+        {
+            if (!msg._callStack.empty() && !_settings.nomsg.isSuppressed(msg._id, msg._callStack.begin()->getfile(), msg._callStack.begin()->line))
+                _next->reportErr(msg);
+        }
+        virtual void reportStatus(unsigned int index, unsigned int max)
+        {
+            _next->reportStatus(index, max);
+        }
+    private:
+        Settings &_settings;
+        ErrorLogger *_next;
+    };
+
+    void check_preprocess_suppress(const char precode[], const char *filename = NULL)
+    {
+        // Clear the error buffer..
+        errout.str("");
+
+        if (filename == NULL)
+            filename = "test.cpp";
+
+        Settings settings;
+        settings._checkCodingStyle = true;
+
+        // Preprocess file..
+        Preprocessor preprocessor(&settings, this);
+        std::list<std::string> configurations;
+        std::string filedata = "";
+        std::istringstream fin(precode);
+        preprocessor.preprocess(fin, filedata, configurations, filename, settings._includePaths);
+        SimpleSuppressor logger(settings, this);
+        const std::string code = Preprocessor::getcode(filedata, "", filename, &settings, &logger);
+
+        // Tokenize..
+        Tokenizer tokenizer(&settings, &logger);
+        std::istringstream istr(code);
+        tokenizer.tokenize(istr, filename);
+        tokenizer.simplifyGoto();
+
+        // Check..
+        CheckOther checkOther(&tokenizer, &settings, &logger);
+        checkOther.checkSwitchCaseFallThrough();
+
+        logger.reportUnmatchedSuppressions(settings.nomsg.getUnmatchedLocalSuppressions(filename));
+    }
+
 
     void zeroDiv1()
     {
@@ -1146,6 +1205,252 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void switchFallThroughCase()
+    {
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            break;\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 0:\n"
+            "        case 1:\n"
+            "            break;\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            g();\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (warning) Switch falls through case without comment\n", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            g();\n"
+            "        default:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (warning) Switch falls through case without comment\n", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            g();\n"
+            "            // fall through\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            g();\n"
+            "            /* FALLTHRU */\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            g();\n"
+            "            break;\n"
+            "            // fall through\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("[test.cpp:7]: (information) Unmatched suppression: switchCaseFallThrough\n", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            {\n"
+            "                break;\n"
+            "            }\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            for (;;) {\n"
+            "                break;\n"
+            "            }\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("[test.cpp:7]: (warning) Switch falls through case without comment\n", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            if (b) {\n"
+            "                break;\n"
+            "            } else {\n"
+            "                break;\n"
+            "            }\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            if (b) {\n"
+            "                break;\n"
+            "            } else {\n"
+            "            }\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("[test.cpp:8]: (warning) Switch falls through case without comment\n", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            if (b) {\n"
+            "                break;\n"
+            "            }\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("[test.cpp:7]: (warning) Switch falls through case without comment\n", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            if (b) {\n"
+            "            } else {\n"
+            "                break;\n"
+            "            }\n"
+            "        case 2:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("[test.cpp:8]: (warning) Switch falls through case without comment\n", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "        case 1:\n"
+            "            if (b) {\n"
+            "        case 2:\n"
+            "            } else {\n"
+            "                break;\n"
+            "            }\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (warning) Switch falls through case without comment\n", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "            int x;\n"
+            "        case 1:\n"
+            "            break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "    case 1:\n"
+            "        g();\n"
+            "        switch (b) {\n"
+            "            case 1:\n"
+            "                return;\n"
+            "            default:\n"
+            "                return;\n"
+            "        }\n"
+            "    case 2:\n"
+            "        break;\n"
+            "    }\n"
+            "}\n");
+        // This fails because the switch parsing code currently doesn't understand
+        // that all paths after g() actually return. It's a pretty unusual case
+        // (no pun intended).
+        TODO_ASSERT_EQUALS("",
+                           "[test.cpp:11]: (warning) Switch falls through case without comment\n", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "    case 1:\n"
+            "#ifndef A\n"
+            "        g();\n"
+            "        // fall through\n"
+            "#endif\n"
+            "    case 2:\n"
+            "        break;\n"
+            "    }\n"
+            "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_preprocess_suppress(
+            "void foo() {\n"
+            "    switch (a) {\n"
+            "    case 1:\n"
+            "        goto leave;\n"
+            "    case 2:\n"
+            "        break;\n"
+            "    }\n"
+            "leave:\n"
+            "    if (x) {\n"
+            "        g();\n"
+            "        return;\n"
+            "    }\n"
+            "}\n");
+        // This fails because Tokenizer::simplifyGoto() copies the "leave:" block
+        // into where the goto is, but because it contains a "return", it omits
+        // copying a final return after the block.
+        TODO_ASSERT_EQUALS("",
+                           "[test.cpp:5]: (warning) Switch falls through case without comment\n", errout.str());
+    }
+
     void selfAssignment()
     {
         check("void foo()\n"