From 93ea774484498d4fd2b00634873ec1f190438d83 Mon Sep 17 00:00:00 2001 From: Greg Hewgill Date: Sat, 19 Feb 2011 21:33:29 +1300 Subject: [PATCH 01/10] initial simplistic implementation of switchCaseFallThrough --- lib/checkother.cpp | 46 +++++++++++++++++++ lib/checkother.h | 6 +++ lib/preprocessor.cpp | 7 ++- test/testother.cpp | 106 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 164 insertions(+), 1 deletion(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 8588e3666..b7e3e6e19 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -306,6 +306,46 @@ void CheckOther::checkRedundantAssignmentInSwitch() } +void CheckOther::checkSwitchCaseFallThrough() +{ + const char switchPattern[] = "switch ( %any% ) { case"; + //const char breakPattern[] = "break|continue|return|exit|goto"; + //const char functionPattern[] = "%var% ("; + // nested switch + + // 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 + for (const Token *tok2 = tok->tokAt(6); tok2; tok2 = tok2->next()) + { + if (Token::Match(tok2, switchPattern)) + { + tok2 = tok2->tokAt(4)->link()->previous(); + } + else if (tok2->str() == "case") + { + if (!Token::Match(tok2->previous()->previous(), "break")) + { + switchCaseFallThrough(tok2); + } + } + else if (tok2->str() == "}") + { + // End of the switch block + break; + } + + } + + tok = Token::findmatch(tok->next(), switchPattern); + } +} + + //--------------------------------------------------------------------------- // int x = 1; // x = x; // <- redundant assignment to self @@ -2998,6 +3038,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..63c7ebed7 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -61,6 +61,7 @@ public: checkOther.sizeofsizeof(); checkOther.sizeofCalculation(); checkOther.checkRedundantAssignmentInSwitch(); + checkOther.checkSwitchCaseFallThrough(); checkOther.checkAssignmentInAssert(); checkOther.checkSizeofForArrayParameter(); checkOther.checkSelfAssignment(); @@ -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..9f4bb0384 100644 --- a/lib/preprocessor.cpp +++ b/lib/preprocessor.cpp @@ -377,10 +377,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 +392,11 @@ std::string Preprocessor::removeComments(const std::string &str, const std::stri } } + if (comment.find("fall through") != std::string::npos) + { + suppressionIDs.push_back("switchCaseFallThrough"); + } + code << "\n"; previous = '\n'; ++lineno; diff --git a/test/testother.cpp b/test/testother.cpp index 5707fc264..43d07ee8b 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,60 @@ 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); + + // Check.. + CheckOther checkOther(&tokenizer, &settings, &logger); + checkOther.checkSwitchCaseFallThrough(); + } + void zeroDiv1() { @@ -1146,6 +1202,56 @@ 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 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" + " // 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" + " break;\n" + " // fall through\n" + " case 2:\n" + " break;\n" + " }\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + } + void selfAssignment() { check("void foo()\n" From a532a9690e4bf9dd8bba42c23d856fb3b96c369b Mon Sep 17 00:00:00 2001 From: Greg Hewgill Date: Sun, 20 Feb 2011 08:02:28 +1300 Subject: [PATCH 02/10] full implementation of switch case fall through --- lib/checkother.cpp | 97 +++++++++++++++++++++++++++++++----- lib/checkother.h | 2 +- test/testother.cpp | 121 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 208 insertions(+), 12 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index b7e3e6e19..02688ce81 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) @@ -308,10 +309,8 @@ void CheckOther::checkRedundantAssignmentInSwitch() void CheckOther::checkSwitchCaseFallThrough() { - const char switchPattern[] = "switch ( %any% ) { case"; - //const char breakPattern[] = "break|continue|return|exit|goto"; - //const char functionPattern[] = "%var% ("; - // nested switch + const char switchPattern[] = "switch ("; + const char breakPattern[] = "break|continue|return|exit|goto"; // Find the beginning of a switch. E.g.: // switch (var) { ... @@ -320,23 +319,99 @@ void CheckOther::checkSwitchCaseFallThrough() { // Check the contents of the switch statement - for (const Token *tok2 = tok->tokAt(6); tok2; tok2 = tok2->next()) + std::stack > ifnest; + std::stack loopnest; + std::stack scopenest; + bool justbreak = true; + for (const Token *tok2 = tok->tokAt(1)->link()->tokAt(2); tok2; tok2 = tok2->next()) { - if (Token::Match(tok2, switchPattern)) + if (Token::Match(tok2, "if (")) { - tok2 = tok2->tokAt(4)->link()->previous(); + tok2 = tok2->tokAt(1)->link()->next(); + ifnest.push(std::make_pair(tok2->link(), false)); + justbreak = false; } - else if (tok2->str() == "case") + else if (Token::Match(tok2, "while (")) { - if (!Token::Match(tok2->previous()->previous(), "break")) + tok2 = tok2->tokAt(1)->link()->next(); + loopnest.push(tok2->link()); + justbreak = false; + } + else if (Token::Match(tok2, "do {")) + { + tok2 = tok2->tokAt(1); + loopnest.push(tok2->link()); + justbreak = false; + } + else if (Token::Match(tok2, "for (")) + { + tok2 = tok2->tokAt(1)->link()->next(); + 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) { switchCaseFallThrough(tok2); } + tok2 = Token::findmatch(tok2, ":"); + justbreak = true; + } + else if (tok2->str() == "{") + { + scopenest.push(tok2->link()); } else if (tok2->str() == "}") { - // End of the switch block - break; + if (!ifnest.empty() && tok2 == ifnest.top().first) + { + if (tok2->next()->str() == "else") + { + tok2 = tok2->tokAt(2); + ifnest.pop(); + 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 + { + assert(ifnest.empty()); + assert(loopnest.empty()); + assert(scopenest.empty()); + // end of switch block + break; + } + } + else if (tok2->str() != ";") + { + justbreak = false; } } diff --git a/lib/checkother.h b/lib/checkother.h index 63c7ebed7..80605333b 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -61,7 +61,6 @@ public: checkOther.sizeofsizeof(); checkOther.sizeofCalculation(); checkOther.checkRedundantAssignmentInSwitch(); - checkOther.checkSwitchCaseFallThrough(); checkOther.checkAssignmentInAssert(); checkOther.checkSizeofForArrayParameter(); checkOther.checkSelfAssignment(); @@ -91,6 +90,7 @@ public: checkOther.checkIncorrectStringCompare(); checkOther.checkIncrementBoolean(); checkOther.checkComparisonOfBoolWithInt(); + checkOther.checkSwitchCaseFallThrough(); } /** @brief Clarify calculation for ".. a * b ? .." */ diff --git a/test/testother.cpp b/test/testother.cpp index 43d07ee8b..c699b78fd 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -202,6 +202,8 @@ private: // Check.. CheckOther checkOther(&tokenizer, &settings, &logger); checkOther.checkSwitchCaseFallThrough(); + + logger.reportUnmatchedSuppressions(settings.nomsg.getUnmatchedLocalSuppressions(filename)); } @@ -1215,6 +1217,18 @@ private: "}\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" @@ -1226,6 +1240,17 @@ private: "}\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" @@ -1249,7 +1274,103 @@ private: " 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()); } void selfAssignment() From ad457378050ef908d09cd4fc312244723b41c0c2 Mon Sep 17 00:00:00 2001 From: Greg Hewgill Date: Sun, 20 Feb 2011 09:43:24 +1300 Subject: [PATCH 03/10] more gracefully handle unexpected blocks inside switch --- lib/checkother.cpp | 52 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 48 insertions(+), 4 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 02688ce81..e3135dd71 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -328,24 +328,56 @@ void CheckOther::checkSwitchCaseFallThrough() 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(); - loopnest.push(tok2->link()); + // 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; } @@ -402,9 +434,21 @@ void CheckOther::checkSwitchCaseFallThrough() } else { - assert(ifnest.empty()); - assert(loopnest.empty()); - assert(scopenest.empty()); + 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; } From 610d2efaeac12586916cd9c0aec1acd05d7f6e64 Mon Sep 17 00:00:00 2001 From: Greg Hewgill Date: Sun, 20 Feb 2011 09:54:01 +1300 Subject: [PATCH 04/10] recognise fall through in c style comments --- lib/preprocessor.cpp | 11 +++++++++-- test/testother.cpp | 12 ++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/preprocessor.cpp b/lib/preprocessor.cpp index 9f4bb0384..c9d125d83 100644 --- a/lib/preprocessor.cpp +++ b/lib/preprocessor.cpp @@ -392,7 +392,8 @@ std::string Preprocessor::removeComments(const std::string &str, const std::stri } } - if (comment.find("fall through") != std::string::npos) + std::transform(comment.begin(), comment.end(), comment.begin(), ::tolower); + if (comment.find("fall") != std::string::npos && comment.find("thr") != std::string::npos) { suppressionIDs.push_back("switchCaseFallThrough"); } @@ -417,10 +418,16 @@ std::string Preprocessor::removeComments(const std::string &str, const std::stri ++lineno; } } + std::string comment(str, commentStart, i - commentStart); + + std::transform(comment.begin(), comment.end(), comment.begin(), ::tolower); + if (comment.find("fall") != std::string::npos && comment.find("thr") != std::string::npos) + { + suppressionIDs.push_back("switchCaseFallThrough"); + } if (settings && settings->_inlineSuppressions) { - std::string comment(str, commentStart, i - commentStart); std::istringstream iss(comment); std::string word; iss >> word; diff --git a/test/testother.cpp b/test/testother.cpp index c699b78fd..b65bd7663 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -1263,6 +1263,18 @@ private: "}\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" From 8c1d7ef3161806294181c530c889f4d0f5d61b0a Mon Sep 17 00:00:00 2001 From: Greg Hewgill Date: Sun, 20 Feb 2011 23:44:18 +1300 Subject: [PATCH 05/10] avoid crash when else condition doesn't have braces to link --- lib/checkother.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index e3135dd71..93480de5e 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -415,6 +415,13 @@ void CheckOther::checkSwitchCaseFallThrough() { 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; } From 1a606a57fd0deeb768fe5fa87f44b33a49a8e898 Mon Sep 17 00:00:00 2001 From: Greg Hewgill Date: Sun, 20 Feb 2011 23:56:52 +1300 Subject: [PATCH 06/10] slightly more flexible detection of 'fall through' comment --- lib/preprocessor.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/lib/preprocessor.cpp b/lib/preprocessor.cpp index c9d125d83..2e8cd86c5 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 @@ -392,8 +404,7 @@ std::string Preprocessor::removeComments(const std::string &str, const std::stri } } - std::transform(comment.begin(), comment.end(), comment.begin(), ::tolower); - if (comment.find("fall") != std::string::npos && comment.find("thr") != std::string::npos) + if (isFallThroughComment(comment)) { suppressionIDs.push_back("switchCaseFallThrough"); } @@ -420,8 +431,7 @@ std::string Preprocessor::removeComments(const std::string &str, const std::stri } std::string comment(str, commentStart, i - commentStart); - std::transform(comment.begin(), comment.end(), comment.begin(), ::tolower); - if (comment.find("fall") != std::string::npos && comment.find("thr") != std::string::npos) + if (isFallThroughComment(comment)) { suppressionIDs.push_back("switchCaseFallThrough"); } From 70fcbe94f49d81438d5ab7276e8e4e6726d11db9 Mon Sep 17 00:00:00 2001 From: Greg Hewgill Date: Wed, 23 Feb 2011 22:45:21 +1300 Subject: [PATCH 07/10] avoid warning on first case (in case there are declarations before first case) --- lib/checkother.cpp | 4 +++- test/testother.cpp | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 93480de5e..8324808c5 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -323,6 +323,7 @@ void CheckOther::checkSwitchCaseFallThrough() 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 (")) @@ -396,12 +397,13 @@ void CheckOther::checkSwitchCaseFallThrough() } else if (Token::Match(tok2, "case|default")) { - if (!justbreak) + if (!justbreak && !firstcase) { switchCaseFallThrough(tok2); } tok2 = Token::findmatch(tok2, ":"); justbreak = true; + firstcase = false; } else if (tok2->str() == "{") { diff --git a/test/testother.cpp b/test/testother.cpp index b65bd7663..e939f0d61 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -1383,6 +1383,16 @@ private: " }\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()); } void selfAssignment() From 8e839a46e8f5d55eb287b26027e45ef6c21a6f08 Mon Sep 17 00:00:00 2001 From: Greg Hewgill Date: Fri, 4 Mar 2011 19:22:17 +1300 Subject: [PATCH 08/10] add TODO for pathological case --- test/testother.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/testother.cpp b/test/testother.cpp index e939f0d61..a95d8f391 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -1393,6 +1393,27 @@ private: " }\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()); } void selfAssignment() From cc7e05a5b0ff92021f49331e6e50ed98e12c0883 Mon Sep 17 00:00:00 2001 From: Greg Hewgill Date: Fri, 4 Mar 2011 19:26:48 +1300 Subject: [PATCH 09/10] fix case where fall through comment precedes preprocessor line --- lib/preprocessor.cpp | 33 +++++++++++++++++++++++---------- test/testother.cpp | 14 ++++++++++++++ 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/lib/preprocessor.cpp b/lib/preprocessor.cpp index 2e8cd86c5..d93ddd6ab 100644 --- a/lib/preprocessor.cpp +++ b/lib/preprocessor.cpp @@ -326,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) @@ -370,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) { @@ -449,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 a95d8f391..16aa5afc1 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -1414,6 +1414,20 @@ private: // (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()); } void selfAssignment() From c5f8a06a97df57a47a2061e48b55e6fa15f19641 Mon Sep 17 00:00:00 2001 From: Greg Hewgill Date: Fri, 4 Mar 2011 20:26:14 +1300 Subject: [PATCH 10/10] add TODO for case where simplifyGoto() does the wrong thing --- test/testother.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/testother.cpp b/test/testother.cpp index 16aa5afc1..7aab2c4da 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -198,6 +198,7 @@ private: Tokenizer tokenizer(&settings, &logger); std::istringstream istr(code); tokenizer.tokenize(istr, filename); + tokenizer.simplifyGoto(); // Check.. CheckOther checkOther(&tokenizer, &settings, &logger); @@ -1428,6 +1429,26 @@ private: " }\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()