Rewrote CheckOther::checkVariableScope()

lib/checkother.cpp

@@ -2052,101 +2052,91 @@ void CheckOther::checkVariableScope()
         } else if ((tok->str() == "=" || tok->str() == "(") &&
                    ((!tok->next()->isNumber() && tok->next()->type() != Token::eString && tok->next()->type() != Token::eChar && !tok->next()->isBoolean()) || tok->strAt(2) != ";"))
             continue;
-        lookupVar(tok, var);
+
+        bool reduce = true;
+        bool used = false; // Don't warn about unused variables
+        for (; tok != var->scope()->classEnd; tok = tok->next()) {
+            if (tok->str() == "{" && tok->strAt(-1) != "=") {
+                if (used || !checkInnerScope(tok, var, used)) {
+                    reduce = false;
+                    break;
+                }
+                tok = tok->link();
+            } else if (tok->varId() == var->varId() || tok->str() == "goto") {
+                reduce = false;
+                break;
+            }
+        }
+
+        if (reduce && used)
+            variableScopeError(var->nameToken(), var->name());
     }
 }
 
-void CheckOther::lookupVar(const Token *tok, const Variable* var)
+bool CheckOther::checkInnerScope(const Token *tok, const Variable* var, bool& used)
 {
-    // Skip the variable declaration..
+    const Scope* scope = tok->next()->scope();
-    while (tok && tok->str() != ";")
+    bool loopVariable = scope->type == Scope::eFor || scope->type == Scope::eWhile || scope->type == Scope::eDo;
-        tok = tok->next();
+    bool header = false;
+    bool noContinue = true;
+    const Token* forHeadEnd = 0;
+    const Token* end = tok->link();
 
-    // Check if the variable is used in this indentlevel..
+    if (scope->type == Scope::eUnconditional && (tok->strAt(-1) == ")" || tok->previous()->isName())) // Might be an unknown macro like BOOST_FOREACH
-    bool used1 = false;   // used in one sub-scope -> reducible
+        loopVariable = true;
-    bool used2 = false;   // used in more sub-scopes -> not reducible
+
-    unsigned int indentlevel = 0;
+    if (scope->type == Scope::eDo) {
-    int parlevel = 0;
+        end = end->linkAt(2);
-    bool for_or_while = false;  // is sub-scope a "for/while/etc". anything that is not "if"
+    } else if (loopVariable && tok->strAt(-1) == ")") {
-    while (tok) {
+        tok = tok->linkAt(-1); // Jump to opening ( of for/while statement
-        if (tok->str() == "{") {
+        header = true;
-            if (tok->strAt(-1) == "=") {
+    } else if (scope->type == Scope::eSwitch)
-                if (Token::findmatch(tok, "%varid%", tok->link(), var->varId())) {
+        return false; // TODO: Support switch properly
-                    return;
+
+    for (; tok != end; tok = tok->next()) {
+        if (tok->str() == "goto")
+            return false;
+        if (tok->str() == "continue")
+            noContinue = false;
+
+        if (Token::simpleMatch(tok, "for ("))
+            forHeadEnd = tok->linkAt(1);
+        if (tok == forHeadEnd)
+            forHeadEnd = 0;
+
+        if (noContinue && tok->scope() == scope && !forHeadEnd && scope->type != Scope::eSwitch && Token::Match(tok, "%varid% =", var->varId())) { // Assigned in outer scope.
+            loopVariable = false;
+            unsigned int indent = 0;
+            for (const Token* tok2 = tok->tokAt(2); tok2; tok2 = tok2->next()) { // Ensure that variable isn't used on right side of =, too
+                if (tok2->str() == "(")
+                    indent++;
+                else if (tok2->str() == ")") {
+                    if (indent == 0)
+                        break;
+                    indent--;
+                } else if (tok2->str() == ";")
+                    break;
+                else if (tok2->varId() == var->varId()) {
+                    loopVariable = true;
+                    break;
                 }
-
-                tok = tok->link();
-            } else
-                ++indentlevel;
-        }
-
-        else if (tok->str() == "}") {
-            if (indentlevel == 0)
-                break;
-            --indentlevel;
-            if (indentlevel == 0) {
-                if (for_or_while && used2)
-                    return;
-                used2 |= used1;
-                used1 = false;
             }
         }
 
-        else if (tok->str() == "(") {
+        if (loopVariable && Token::Match(tok, "%varid% !!=", var->varId())) // Variable used in loop
-            ++parlevel;
+            return false;
-        }
 
-        else if (tok->str() == ")") {
+        if (Token::Match(tok, "& %varid%", var->varId())) // Taking address of variable
-            --parlevel;
+            return false;
-        }
 
-        // Bail out if references are used
+        if (Token::Match(tok, "= %varid%", var->varId()) && (var->isArray() || var->isPointer())) // Create a copy of array/pointer. Bailout, because the memory it points to might be necessary in outer scope
-        else if (Token::Match(tok, "& %varid%", var->varId())) {
+            return false;
-            return;
-        }
 
-        else if (tok->varId() == var->varId()) {
+        if (tok->varId() == var->varId())
-            if (indentlevel == 0)
+            used = true;
-                return;
-            if (tok->strAt(-1) == "=" && (var->isArray() || var->isPointer())) // Create a copy of array/pointer. Bailout, because the memory it points to might be necessary in outer scope
-                return;
-            used1 = true;
-            if (for_or_while && tok->strAt(1) != "=")
-                used2 = true;
-            if (used1 && used2)
-                return;
-        }
-
-        else if (indentlevel == 0) {
-            // %unknown% ( %any% ) {
-            // If %unknown% is anything except if, we assume
-            // that it is a for or while loop or a macro hiding either one
-            if (tok->strAt(1) == "(" &&
-                Token::simpleMatch(tok->next()->link(), ") {")) {
-                if (tok->str() != "if")
-                    for_or_while = true;
-            }
-
-            else if (Token::simpleMatch(tok, "do {"))
-                for_or_while = true;
-
-            // possible unexpanded macro hiding for/while..
-            else if (tok->str() != "else" && Token::Match(tok->previous(), "[;{}] %type% {")) {
-                for_or_while = true;
-            }
-
-            if (parlevel == 0 && (tok->str() == ";"))
-                for_or_while = false;
-        }
-
-        tok = tok->next();
     }
 
-    // Warning if this variable:
+    return true;
-    // * not used in this indentlevel
-    // * used in lower indentlevel
-    if (used1 || used2)
-        variableScopeError(var->nameToken(), var->name());
 }
 
 void CheckOther::variableScopeError(const Token *tok, const std::string &varname)

lib/checkother.h

@@ -154,7 +154,7 @@ public:
 
     /** @brief %Check scope of variables */
     void checkVariableScope();
-    void lookupVar(const Token *tok, const Variable* var);
+    bool checkInnerScope(const Token *tok, const Variable* var, bool& used);
 
     /** @brief %Check for constant function parameter */
     void checkConstantFunctionParameter();

test/testother.cpp

@@ -68,6 +68,7 @@ private:
         TEST_CASE(varScope13);      // variable usage in inner loop
         TEST_CASE(varScope14);
         TEST_CASE(varScope15);      // #4573 if-else-if
+        TEST_CASE(varScope16);
 
         TEST_CASE(oldStylePointerCast);
         TEST_CASE(invalidPointerCast);
@@ -865,6 +866,42 @@ private:
         ASSERT_EQUALS("", errout.str());
     }
 
+    void varScope16() {
+        varScope("void f() {\n"
+                 "    int a = 0;\n"
+                 "    while((++a) < 56) {\n"
+                 "        foo();\n"
+                 "    }\n"
+                 "}");
+        ASSERT_EQUALS("", errout.str());
+
+        varScope("void f() {\n"
+                 "    int a = 0;\n"
+                 "    do {\n"
+                 "        foo();\n"
+                 "    } while((++a) < 56);\n"
+                 "}");
+        ASSERT_EQUALS("", errout.str());
+
+        varScope("void f() {\n"
+                 "    int a = 0;\n"
+                 "    do {\n"
+                 "        a = 64;\n"
+                 "        foo(a);\n"
+                 "    } while((++a) < 56);\n"
+                 "}");
+        ASSERT_EQUALS("", errout.str());
+
+        varScope("void f() {\n"
+                 "    int a = 0;\n"
+                 "    do {\n"
+                 "        a = 64;\n"
+                 "        foo(a);\n"
+                 "    } while(z());\n"
+                 "}");
+        ASSERT_EQUALS("[test.cpp:2]: (style) The scope of the variable 'a' can be reduced.\n", errout.str());
+    }
+
     void checkOldStylePointerCast(const char code[]) {
         // Clear the error buffer..
         errout.str("");