STL: refactoring CheckStl::erase so ExecutionPath is used

lib/checkstl.cpp

@@ -18,7 +18,7 @@
 
 #include "checkstl.h"
 #include "token.h"
-
+#include "executionpath.h"
 
 
 // Register this check class (by creating a static instance of it)
@@ -221,6 +221,134 @@ void CheckStl::stlOutOfBoundsError(const Token *tok, const std::string &num, con
 }
 
 
+/**
+ * @brief %Check for invalid iterator usage after erase/insert/etc
+ */
+class EraseCheckLoop : public ExecutionPath
+{
+public:
+    static void checkScope(CheckStl *checkStl, const Token *it)
+    {
+        const Token *tok = it;
+
+        // Search for the start of the loop body..
+        int indentlevel = 1;
+        while (indentlevel > 0 && 0 != (tok = tok->next()))
+        {
+            if (tok->str() == "(")
+                ++indentlevel;
+            else if (tok->str() == ")")
+                --indentlevel;
+        }
+
+        if (! Token::simpleMatch(tok, ") {"))
+            return;
+
+        EraseCheckLoop c(checkStl, it->varId());
+        std::list<ExecutionPath *> checks;
+        checks.push_back(c.copy());
+        ExecutionPath::checkScope(tok->tokAt(2), checks);
+
+        c.end(checks, tok->link());
+
+        while (!checks.empty())
+        {
+            delete checks.back();
+            checks.pop_back();
+        }
+    }
+
+private:
+    /** Startup constructor */
+    EraseCheckLoop(Check *o, unsigned int varid)
+        : ExecutionPath(o, varid), eraseToken(0)
+    {
+    }
+
+    /** @brief token where iterator is erased (non-zero => the iterator is invalid) */
+    const Token *eraseToken;
+
+    /** @brief Copy this check. Called from the ExecutionPath baseclass. */
+    ExecutionPath *copy()
+    {
+        return new EraseCheckLoop(*this);
+    }
+
+    /** @brief is another execution path equal? */
+    bool is_equal(const ExecutionPath *e) const
+    {
+        const EraseCheckLoop *c = static_cast<const EraseCheckLoop *>(e);
+        return (eraseToken == c->eraseToken);
+    }
+
+    /** @brief no implementation => compiler error if used by accident */
+    void operator=(const EraseCheckLoop &);
+
+    /** @brief parse tokens */
+    const Token *parse(const Token &tok, std::list<ExecutionPath *> &checks) const
+    {
+        if (Token::Match(&tok, "[;{}] %var% =") || Token::Match(&tok, "= %var% ;"))
+        {
+            ExecutionPath::bailOutVar(checks, tok.next()->varId());
+        }
+
+        if (Token::Match(&tok, "[;{}] break ;"))
+        {
+            ExecutionPath::bailOut(checks);
+        }
+
+        if (Token::Match(&tok, "erase ( ++|--| %var% )"))
+        {
+            const Token *token = &tok;
+            while (NULL != (token = token ? token->previous() : 0))
+            {
+                if (Token::Match(token, "[;{}]"))
+                    break;
+                else if (token->str() == "=")
+                    token = 0;
+                else
+                    token = token->previous();
+            }
+
+            if (token)
+            {
+                unsigned int iteratorId = 0;
+                if (tok.tokAt(2)->isName())
+                    iteratorId = tok.tokAt(2)->varId();
+                else
+                    iteratorId = tok.tokAt(3)->varId();
+
+                for (std::list<ExecutionPath *>::const_iterator it = checks.begin(); it != checks.end(); ++it)
+                {
+                    EraseCheckLoop *c = dynamic_cast<EraseCheckLoop *>(*it);
+                    if (c && c->varId == iteratorId)
+                    {
+                        c->eraseToken = &tok;
+                    }
+                }
+            }
+        }
+        return &tok;
+    }
+
+    /** @brief going out of scope - all execution paths end */
+    void end(const std::list<ExecutionPath *> &checks, const Token * /*tok*/) const
+    {
+        for (std::list<ExecutionPath *>::const_iterator it = checks.begin(); it != checks.end(); ++it)
+        {
+            EraseCheckLoop *c = dynamic_cast<EraseCheckLoop *>(*it);
+            if (c && c->eraseToken)
+            {
+                CheckStl *checkStl = dynamic_cast<CheckStl *>(c->owner);
+                if (checkStl)
+                {
+                    checkStl->eraseError(c->eraseToken);
+                }
+            }
+        }
+    }
+};
+
 
 void CheckStl::erase()
 {
@@ -236,7 +364,7 @@ void CheckStl::erase()
                     {
                         const unsigned int varid = tok2->next()->varId();
                         if (varid > 0 && Token::findmatch(_tokenizer->tokens(), "> :: iterator %varid%", varid))
-                            eraseCheckLoop(tok2->next());
+                            EraseCheckLoop::checkScope(this, tok2->next());
                     }
                     break;
                 }
@@ -245,7 +373,7 @@ void CheckStl::erase()
                     tok2->str() == tok2->tokAt(8)->str() &&
                     tok2->tokAt(2)->str() == tok2->tokAt(10)->str())
                 {
-                    eraseCheckLoop(tok2);
+                    EraseCheckLoop::checkScope(this, tok2);
                     break;
                 }
             }
@@ -255,71 +383,11 @@ void CheckStl::erase()
         {
             const unsigned int varid = tok->tokAt(2)->varId();
             if (varid > 0 && Token::findmatch(_tokenizer->tokens(), "> :: iterator %varid%", varid))
-                eraseCheckLoop(tok->tokAt(2));
+                EraseCheckLoop::checkScope(this, tok->tokAt(2));
         }
     }
 }
 
-void CheckStl::eraseCheckLoop(const Token *it)
-{
-    const Token *tok = it;
-
-    // Search for the start of the loop body..
-    int indentlevel = 1;
-    while (indentlevel > 0 && 0 != (tok = tok->next()))
-    {
-        if (tok->str() == "(")
-            ++indentlevel;
-        else if (tok->str() == ")")
-            --indentlevel;
-    }
-
-    if (! Token::simpleMatch(tok, ") {"))
-        return;
-
-    // Parse loop..
-    // Error if it contains "erase(it)" but neither "break;", "=it" nor "it="
-    indentlevel = 0;
-    const Token *tok2 = 0;
-    while (0 != (tok = tok->next()))
-    {
-        if (tok->str() == "{")
-            ++indentlevel;
-        else if (tok->str() == "}")
-        {
-            --indentlevel;
-            if (indentlevel <= 0)
-                break;
-        }
-        else if (Token::Match(tok, "break|return|goto") ||
-                 Token::simpleMatch(tok, (it->str() + " =").c_str()) ||
-                 Token::simpleMatch(tok, ("= " + it->str()).c_str()))
-        {
-            tok2 = 0;
-            break;
-        }
-        else if (Token::Match(tok, ("erase ( ++|--| " + it->str() + " )").c_str()))
-        {
-            tok2 = tok;
-            while (NULL != (tok2 = tok2 ? tok2->previous() : 0))
-            {
-                if (Token::Match(tok2, "[;{}]"))
-                    break;
-                else if (tok2->str() == "=")
-                    tok2 = 0;
-                else
-                    tok2 = tok2->previous();
-            }
-            if (tok2)
-                tok2 = tok;
-        }
-    }
-
-    // Write error message..
-    if (tok2)
-        eraseError(tok2);
-}
-
 
 // Error message for bad iterator usage..
 void CheckStl::eraseError(const Token *tok)

lib/checkstl.h

@@ -86,6 +86,8 @@ public:
      * Dangerous usage of erase
      */
     void erase();
+    void eraseError(const Token *tok);
+
 
     /**
      * Dangerous usage of push_back and insert
@@ -124,7 +126,6 @@ private:
     void invalidIteratorError(const Token *tok, const std::string &iteratorName);
     void iteratorsError(const Token *tok, const std::string &container1, const std::string &container2);
     void mismatchingContainersError(const Token *tok);
-    void eraseError(const Token *tok);
     void invalidIteratorError(const Token *tok, const std::string &func, const std::string &iterator_name);
     void invalidPointerError(const Token *tok, const std::string &pointer_name);
     void stlBoundriesError(const Token *tok, const std::string &container_name);

lib/executionpath.cpp

@@ -25,8 +25,6 @@
 #include <iostream>
 
 
-static void checkExecutionPaths_(const Token *tok, std::list<ExecutionPath *> &checks);
-
 
 // default : bail out if the condition is has variable handling
 bool ExecutionPath::parseCondition(const Token &tok, std::list<ExecutionPath *> & checks)
@@ -111,7 +109,7 @@ static void parseIfSwitchBody(const Token * const tok,
                 countif2.insert((*it)->varId);
         }
     }
-    checkExecutionPaths_(tok, c);
+    ExecutionPath::checkScope(tok, c);
     while (!c.empty())
     {
         if (c.back()->varId == 0)
@@ -141,7 +139,7 @@ static void parseIfSwitchBody(const Token * const tok,
 }
 
 
-static void checkExecutionPaths_(const Token *tok, std::list<ExecutionPath *> &checks)
+void ExecutionPath::checkScope(const Token *tok, std::list<ExecutionPath *> &checks)
 {
     if (!tok || tok->str() == "}" || checks.empty())
         return;
@@ -345,7 +343,7 @@ static void checkExecutionPaths_(const Token *tok, std::list<ExecutionPath *> &c
         // ; { ... }
         if (Token::Match(tok->previous(), "[;{}] {"))
         {
-            checkExecutionPaths_(tok->next(), checks);
+            ExecutionPath::checkScope(tok->next(), checks);
             tok = tok->link();
             continue;
         }
@@ -398,7 +396,7 @@ static void checkExecutionPaths_(const Token *tok, std::list<ExecutionPath *> &c
                     continue;
 
                 // there is no "if"..
-                checkExecutionPaths_(tok->next(), checks);
+                ExecutionPath::checkScope(tok->next(), checks);
                 tok = tok->link();
                 if (!tok)
                 {
@@ -454,7 +452,7 @@ void checkExecutionPaths(const Token *tok, ExecutionPath *c)
 
             std::list<ExecutionPath *> checks;
             checks.push_back(c->copy());
-            checkExecutionPaths_(tok, checks);
+            ExecutionPath::checkScope(tok, checks);
 
             c->end(checks, tok->link());
 

lib/executionpath.h

@@ -120,9 +120,12 @@ public:
     {
         return bool(varId == e.varId && is_equal(&e));
     }
+
+    static void checkScope(const Token *tok, std::list<ExecutionPath *> &checks);
 };
 
 
+void checkExecutionPaths(const Token *tok, ExecutionPath *c);
 void checkExecutionPaths(const Token *tok, ExecutionPath *c);
 
 

test/testother.cpp

@@ -3160,13 +3160,13 @@ private:
     void trac2071()
     {
         check("void f() {\n"
-            "    struct AB {\n"
+              "    struct AB {\n"
-            "        AB(int a) { }\n"
+              "        AB(int a) { }\n"
-            "    };\n"
+              "    };\n"
-            "\n"
+              "\n"
-            "    const AB ab[3] = { AB(0), AB(1), AB(2) };\n"
+              "    const AB ab[3] = { AB(0), AB(1), AB(2) };\n"
-            "}\n"
+              "}\n"
-            );
+             );
         ASSERT_EQUALS("", errout.str());
     }
 };

test/teststl.cpp

@@ -384,7 +384,7 @@ private:
     {
         check("static void f()\n"
               "{\n"
-              "    for (it = foo.begin(); it != foo.end(); it = next)\n"
+              "    for (iterator it = foo.begin(); it != foo.end(); it = next)\n"
               "    {\n"
               "        next = it;\n"
               "        next++;\n"
@@ -452,10 +452,23 @@ private:
     {
         check("void f()\n"
               "{\n"
-              "    for (it = foo.begin(); it != foo.end(); ++it)\n"
+              "    for (iterator it = foo.begin(); it != foo.end(); ++it)\n"
               "    {\n"
               "        foo.erase(it);\n"
-              "        break;\n"
+              "        if (x)"
+              "            break;\n"
+              "    }\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (error) Dangerous iterator usage. After erase the iterator is invalid so dereferencing it or comparing it with another iterator is invalid.\n", errout.str());
+
+        check("void f()\n"
+              "{\n"
+              "    for (iterator it = foo.begin(); it != foo.end(); ++it)\n"
+              "    {\n"
+              "        if (x) {\n"
+              "            foo.erase(it);\n"
+              "            break;\n"
+              "        }\n"
               "    }\n"
               "}\n");
         ASSERT_EQUALS("", errout.str());
@@ -555,7 +568,7 @@ private:
     {
         check("void f()\n"
               "{\n"
-              "    for (it = foo.begin(); it != foo.end(); ++it)\n"
+              "    for (iterator it = foo.begin(); it != foo.end(); ++it)\n"
               "    {\n"
               "        foo.erase(it);\n"
               "        goto abc;\n"
@@ -569,7 +582,7 @@ private:
     {
         check("void f()\n"
               "{\n"
-              "    for (it = foo.begin(); it != foo.end(); ++it)\n"
+              "    for (iterator it = foo.begin(); it != foo.end(); ++it)\n"
               "    {\n"
               "        foo.erase(it);\n"
               "        it = foo.begin();\n"