diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 94f3e21e8..8d4de08b5 100644 --- a/lib/checkother.cpp +++ b/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.. - while (tok && tok->str() != ";") - tok = tok->next(); + const Scope* scope = tok->next()->scope(); + bool loopVariable = scope->type == Scope::eFor || scope->type == Scope::eWhile || scope->type == Scope::eDo; + bool header = false; + bool noContinue = true; + const Token* forHeadEnd = 0; + const Token* end = tok->link(); - // Check if the variable is used in this indentlevel.. - bool used1 = false; // used in one sub-scope -> reducible - bool used2 = false; // used in more sub-scopes -> not reducible - unsigned int indentlevel = 0; - int parlevel = 0; - bool for_or_while = false; // is sub-scope a "for/while/etc". anything that is not "if" - while (tok) { - if (tok->str() == "{") { - if (tok->strAt(-1) == "=") { - if (Token::findmatch(tok, "%varid%", tok->link(), var->varId())) { - return; + if (scope->type == Scope::eUnconditional && (tok->strAt(-1) == ")" || tok->previous()->isName())) // Might be an unknown macro like BOOST_FOREACH + loopVariable = true; + + if (scope->type == Scope::eDo) { + end = end->linkAt(2); + } else if (loopVariable && tok->strAt(-1) == ")") { + tok = tok->linkAt(-1); // Jump to opening ( of for/while statement + header = true; + } else if (scope->type == Scope::eSwitch) + return false; // TODO: Support switch properly + + 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() == "(") { - ++parlevel; - } + if (loopVariable && Token::Match(tok, "%varid% !!=", var->varId())) // Variable used in loop + return false; - else if (tok->str() == ")") { - --parlevel; - } + if (Token::Match(tok, "& %varid%", var->varId())) // Taking address of variable + return false; - // Bail out if references are used - else if (Token::Match(tok, "& %varid%", var->varId())) { - return; - } + 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 + return false; - else if (tok->varId() == var->varId()) { - if (indentlevel == 0) - 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(); + if (tok->varId() == var->varId()) + used = true; } - // Warning if this variable: - // * not used in this indentlevel - // * used in lower indentlevel - if (used1 || used2) - variableScopeError(var->nameToken(), var->name()); + return true; } void CheckOther::variableScopeError(const Token *tok, const std::string &varname) diff --git a/lib/checkother.h b/lib/checkother.h index edbeea45b..f9b2c38b5 100644 --- a/lib/checkother.h +++ b/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(); diff --git a/test/testother.cpp b/test/testother.cpp index d4468a020..f64a2cd04 100644 --- a/test/testother.cpp +++ b/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("");