Refactoring suppressions

lib/preprocessor.cpp

@@ -32,6 +32,11 @@
 #include <iterator> // back_inserter
 #include <utility>
 
+static bool sameline(const simplecpp::Token *tok1, const simplecpp::Token *tok2)
+{
+    return tok1 && tok2 && tok1->location.sameline(tok2->location);
+}
+
 /**
  * Remove heading and trailing whitespaces from the input parameter.
  * If string is all spaces/tabs, return empty string.
@@ -76,42 +81,42 @@ namespace {
     };
 }
 
-static void parseCommentToken(const simplecpp::Token *tok, std::list<Suppressions::Suppression> &inlineSuppressions, std::list<BadInlineSuppression> *bad)
+static bool parseInlineSuppressionCommentToken(const simplecpp::Token *tok, std::list<Suppressions::Suppression> &inlineSuppressions, std::list<BadInlineSuppression> *bad)
 {
-    if (tok->str().size() < 19)
+    const std::string &comment = tok->str();
-        return;
+    if (comment.size() < 19)
-    const std::string::size_type pos1 = tok->str().find_first_not_of("/* \t");
+        return false;
+    const std::string::size_type pos1 = comment.find_first_not_of("/* \t");
     if (pos1 == std::string::npos)
-        return;
+        return false;
-    if (pos1 + 17 >= tok->str().size())
+    if (pos1 + 17 >= comment.size())
-        return;
+        return false;
-    if (tok->str().compare(pos1, 17, "cppcheck-suppress") != 0)
+    if (comment.compare(pos1, 17, "cppcheck-suppress") != 0)
-        return;
+        return false;
 
     // skip spaces after "cppcheck-suppress"
-    const std::string::size_type pos2 = tok->str().find_first_not_of(" ", pos1+17);
+    const std::string::size_type pos2 = comment.find_first_not_of(" ", pos1+17);
     if (pos2 == std::string::npos)
-        return;
+        return false;
 
-    if (tok->str()[pos2] == '[') {
+    if (comment[pos2] == '[') {
         // multi suppress format
         std::string errmsg;
-        std::vector<Suppressions::Suppression> ss;
+        std::vector<Suppressions::Suppression> suppressions = Suppressions::parseMultiSuppressComment(comment, &errmsg);
-        ss = Suppressions::parseMultiSuppressComment(tok->str(), &errmsg);
 
         if (!errmsg.empty())
             bad->push_back(BadInlineSuppression(tok->location, errmsg));
 
-        for (std::vector<Suppressions::Suppression>::iterator iter = ss.begin(); iter!=ss.end(); ++iter) {
+        for (const Suppressions::Suppression &s : suppressions) {
-            if (!(*iter).errorId.empty())
+            if (!s.errorId.empty())
-                inlineSuppressions.push_back(*iter);
+                inlineSuppressions.push_back(s);
         }
     } else {
         //single suppress format
         std::string errmsg;
         Suppressions::Suppression s;
-        if (!s.parseComment(tok->str(), &errmsg))
+        if (!s.parseComment(comment, &errmsg))
-            return;
+            return false;
 
         if (!s.errorId.empty())
             inlineSuppressions.push_back(s);
@@ -119,37 +124,36 @@ static void parseCommentToken(const simplecpp::Token *tok, std::list<Suppression
         if (!errmsg.empty())
             bad->push_back(BadInlineSuppression(tok->location, errmsg));
     }
+
+    return true;
 }
 
 static void inlineSuppressions(const simplecpp::TokenList &tokens, Settings &mSettings, std::list<BadInlineSuppression> *bad)
 {
-    const simplecpp::Token *current_non_comment_tok;
+    for (const simplecpp::Token *tok = tokens.cfront(); tok; tok = tok ? tok->next : nullptr) {
-    std::list<Suppressions::Suppression> inlineSuppressions;
+        if (!tok->comment)
+            continue;
 
-    for (const simplecpp::Token *tok = tokens.cfront(); tok; /*none*/) {
+        std::list<Suppressions::Suppression> inlineSuppressions;
-        //parse all comment before current non-comment tok
+        if (!parseInlineSuppressionCommentToken(tok, inlineSuppressions, bad))
-        if (tok->comment) {
+            continue;
-            parseCommentToken(tok, inlineSuppressions, bad);
+
+        if (!sameline(tok->previous, tok)) {
+            // find code after comment..
             tok = tok->next;
-            continue;
+            while (tok && tok->comment) {
-        }
+                parseInlineSuppressionCommentToken(tok, inlineSuppressions, bad);
-
+                tok = tok->next;
-        current_non_comment_tok = tok;
+            }
-
+            if (!tok)
-        //parse all comment tok after current non-comment tok in the same line
-        for (tok = tok->next; tok; tok = tok->next) {
-            if ((tok->location.line != current_non_comment_tok->location.line) || !(tok->comment))
                 break;
-            parseCommentToken(tok, inlineSuppressions, bad);
         }
 
-        //if there is no suppress, jump it!
+        if (inlineSuppressions.empty())
-        if (inlineSuppressions.empty()) {
             continue;
-        }
 
         // Relative filename
-        std::string relativeFilename(current_non_comment_tok->location.file());
+        std::string relativeFilename(tok->location.file());
         if (mSettings.relativePaths) {
             for (const std::string & basePath : mSettings.basePaths) {
                 const std::string bp = basePath + "/";
@@ -163,10 +167,9 @@ static void inlineSuppressions(const simplecpp::TokenList &tokens, Settings &mSe
         // Add the suppressions.
         for (Suppressions::Suppression &suppr : inlineSuppressions) {
             suppr.fileName = relativeFilename;
-            suppr.lineNumber = current_non_comment_tok->location.line;
+            suppr.lineNumber = tok->location.line;
             mSettings.nomsg.addSuppression(suppr);
         }
-        inlineSuppressions.clear();
     }
 }
 
@@ -219,11 +222,6 @@ void Preprocessor::setDirectives(const simplecpp::TokenList &tokens)
     }
 }
 
-static bool sameline(const simplecpp::Token *tok1, const simplecpp::Token *tok2)
-{
-    return tok1 && tok2 && tok1->location.sameline(tok2->location);
-}
-
 static std::string readcondition(const simplecpp::Token *iftok, const std::set<std::string> &defined, const std::set<std::string> &undefined)
 {
     const simplecpp::Token *cond = iftok->next;

lib/suppressions.cpp

@@ -112,57 +112,31 @@ std::string Suppressions::parseXmlFile(const char *filename)
     return "";
 }
 
-std::vector<Suppressions::Suppression> Suppressions::parseMultiSuppressComment(std::string comment, std::string *errorMessage)
+std::vector<Suppressions::Suppression> Suppressions::parseMultiSuppressComment(const std::string &comment, std::string *errorMessage)
 {
     std::vector<Suppression> suppressions;
-    if (comment.size() < 2)
-        return suppressions;
 
-    const std::size_t first_non_blank_position = comment.find_first_not_of(" \t\n", 2);
+    // If this function is called we assume that comment starts with "cppcheck-suppress[".
-    const std::size_t suppress_position = comment.find("cppcheck-suppress", 2);
+    const std::string::size_type start_position = comment.find("[");
-    if (suppress_position == std::string::npos)
+    const std::string::size_type end_position = comment.find("]", start_position);
-        return suppressions;
+    if (end_position == std::string::npos) {
-    if (suppress_position != first_non_blank_position) {
-        if (errorMessage && errorMessage->empty())
-            *errorMessage = "Bad multi suppression '" + comment + "'. there shouldn't have non-blank character before cppcheck-suppress";
-        return suppressions;
-    }
-
-    const std::size_t start_position = comment.find("[", suppress_position);
-    const std::size_t end_position = comment.find("]", suppress_position);
-    if (start_position == std::string::npos
-        || end_position == std::string::npos
-        || start_position != suppress_position+17 //there must be no space before "["
-        || start_position >= end_position) {
         if (errorMessage && errorMessage->empty())
             *errorMessage = "Bad multi suppression '" + comment + "'. legal format is cppcheck-suppress[errorId, errorId symbolName=arr, ...]";
         return suppressions;
     }
 
-    //extract supressions
+    // parse all suppressions
-    std::size_t current_comma_position = 0;
+    for (std::string::size_type pos = start_position; pos < end_position;) {
-    //multi_suppressions_word maybe "[errorId1, errorId2 symbolName=arr", who just has left bracket
+        const std::string::size_type pos1 = pos + 1;
-    std::string multi_suppressions_word = comment.substr(start_position, end_position-start_position);
+        pos = comment.find(",", pos1);
-    do {
+        const std::string::size_type pos2 = (pos < end_position) ? pos : end_position;
-        std::string suppression_word;
+        if (pos1 == pos2)
+            continue;
 
-        //single suppression word
-        const std::size_t previous_comma_position=current_comma_position;
-        current_comma_position=multi_suppressions_word.find(",", previous_comma_position+1);  //find "," after previous comma
-        if (current_comma_position == std::string::npos) {
-            suppression_word = multi_suppressions_word.substr(previous_comma_position+1);
-        } else {
-            suppression_word = multi_suppressions_word.substr(previous_comma_position+1, current_comma_position-previous_comma_position-1);
-        }
-
-        //parse single suppression word
         Suppression s;
-        std::string word;
+        std::istringstream iss(comment.substr(pos1, pos2-pos1));
-        std::string errorId;
-        std::string symbolName;
-        std::istringstream iss(suppression_word);
 
-        iss >> errorId;
+        iss >> s.errorId;
         if (!iss) {
             if (errorMessage && errorMessage->empty())
                 *errorMessage = "Bad multi suppression '" + comment + "'. legal format is cppcheck-suppress[errorId, errorId symbolName=arr, ...]";
@@ -171,13 +145,14 @@ std::vector<Suppressions::Suppression> Suppressions::parseMultiSuppressComment(s
         }
 
         while (iss) {
+            std::string word;
             iss >> word;
             if (!iss)
                 break;
             if (word.find_first_not_of("+-*/%#;") == std::string::npos)
                 break;
             if (word.compare(0, 11, "symbolName=") == 0) {
-                symbolName = word.substr(11);
+                s.symbolName = word.substr(11);
             } else {
                 if (errorMessage && errorMessage->empty())
                     *errorMessage = "Bad multi suppression '" + comment + "'. legal format is cppcheck-suppress[errorId, errorId symbolName=arr, ...]";
@@ -186,11 +161,8 @@ std::vector<Suppressions::Suppression> Suppressions::parseMultiSuppressComment(s
             }
         }
 
-        s.errorId = errorId;
-        s.symbolName = symbolName;
         suppressions.push_back(s);
-
+    }
-    } while (current_comma_position!=std::string::npos);
 
     return suppressions;
 }

lib/suppressions.h

@@ -122,7 +122,7 @@ public:
      * @param errorMessage output parameter for error message (wrong suppression attribute)
      * @return empty vector if something wrong.
      */
-    static std::vector<Suppression> parseMultiSuppressComment(std::string comment, std::string *errorMessage);
+    static std::vector<Suppression> parseMultiSuppressComment(const std::string &comment, std::string *errorMessage);
 
     /**
      * @brief Don't show the given error.

test/testsuppressions.cpp

@@ -576,15 +576,10 @@ private:
         ASSERT_EQUALS("arr", suppressions[1].symbolName);
         ASSERT_EQUALS("", errMsg);
 
-        errMsg = "";
-        suppressions=ss.parseMultiSuppressComment("// cppcheck-suppress [errorId]", &errMsg);
-        ASSERT_EQUALS(0, suppressions.size());
-        ASSERT_EQUALS(false, errMsg.empty());
-
         errMsg = "";
         suppressions=ss.parseMultiSuppressComment("// cppcheck-suppress[]", &errMsg);
         ASSERT_EQUALS(0, suppressions.size());
-        ASSERT_EQUALS(false, errMsg.empty());
+        ASSERT_EQUALS(true, errMsg.empty());
 
         errMsg = "";
         suppressions=ss.parseMultiSuppressComment("// cppcheck-suppress[errorId", &errMsg);
@@ -622,23 +617,11 @@ private:
         ASSERT_EQUALS(2, suppressions.size());
         ASSERT_EQUALS(true, errMsg.empty());
 
-        //error, shouldn't have "some text" at beginning
-        errMsg = "";
-        suppressions=ss.parseMultiSuppressComment("//some text cppcheck-suppress[errorId1, errorId2 symbolName=arr]", &errMsg);
-        ASSERT_EQUALS(0, suppressions.size());
-        ASSERT_EQUALS(false, errMsg.empty());
-
         errMsg = "";
         suppressions=ss.parseMultiSuppressComment("//cppcheck-suppress[errorId1, errorId2 symbolName=arr] some text", &errMsg);
         ASSERT_EQUALS(2, suppressions.size());
         ASSERT_EQUALS(true, errMsg.empty());
 
-        //error, shouldn't have "some text" at beginning
-        errMsg = "";
-        suppressions=ss.parseMultiSuppressComment("//some text cppcheck-suppress[errorId1, errorId2 symbolName=arr] some text", &errMsg);
-        ASSERT_EQUALS(0, suppressions.size());
-        ASSERT_EQUALS(false, errMsg.empty());
-
         errMsg = "";
         suppressions=ss.parseMultiSuppressComment("/*cppcheck-suppress[errorId1, errorId2 symbolName=arr]*/", &errMsg);
         ASSERT_EQUALS(2, suppressions.size());