diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 8588e3666..8324808c5 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -24,6 +24,7 @@ #include // std::isupper #include // fabs() +#include //--------------------------------------------------------------------------- // 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 > ifnest; + std::stack loopnest; + std::stack 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, diff --git a/lib/checkother.h b/lib/checkother.h index d43c4c413..80605333b 100644 --- a/lib/checkother.h +++ b/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); diff --git a/lib/preprocessor.cpp b/lib/preprocessor.cpp index 45ed8a425..d93ddd6ab 100644 --- a/lib/preprocessor.cpp +++ b/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 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 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! - // Add any pending inline suppressions that have accumulated. - if (!suppressionIDs.empty()) + if (!inPreprocessorLine) { - 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. - for (size_t j(0); j < suppressionIDs.size(); ++j) + if (settings != NULL) { - const std::string errmsg(settings->nomsg.addSuppression(suppressionIDs[j], filename, lineno)); - if (!errmsg.empty()) + // Add the suppressions. + 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.. diff --git a/test/testother.cpp b/test/testother.cpp index 5707fc264..7aab2c4da 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -16,6 +16,7 @@ * along with this program. If not, see . */ +#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 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"