From b59d79c30357faeba5400a5a148058abaa18e720 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Thu, 18 Feb 2010 18:45:13 +0100 Subject: [PATCH] readded checks for unused variables and unreachable code that were removed in 42c608b6f085693f9bc80bf9be9a3570f0a5bf87 --- lib/checkother.cpp | 197 ++++++++++++++++++++++++++++++++++++ lib/checkother.h | 12 +++ test/testother.cpp | 3 +- test/testunusedvar.cpp | 222 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 433 insertions(+), 1 deletion(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 979d3a75c..31032639b 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -370,6 +370,203 @@ void CheckOther::checkUnsignedDivision() + + +//--------------------------------------------------------------------------- +// Unreachable code below a 'return' +//--------------------------------------------------------------------------- + +void CheckOther::unreachableCode() +{ + const Token *tok = _tokenizer->tokens(); + while ((tok = Token::findmatch(tok, "[;{}] return"))) + { + // Goto the 'return' token + tok = tok->next(); + + // Locate the end of the 'return' statement + while (tok && tok->str() != ";") + tok = tok->next(); + while (tok && tok->next() && tok->next()->str() == ";") + tok = tok->next(); + + if (!tok) + break; + + // If there is a statement below the return it is unreachable +/* original: + if (!Token::Match(tok, "; case|default|}|#") && + !Token::Match(tok, "; %var% :") && + !Token::simpleMatch(tok, "; break")) +*/ + if (Token::simpleMatch(tok, "; break")) + { + unreachableCodeError(tok->next()); + } + } +} + +void CheckOther::unreachableCodeError(const Token *tok) +{ + reportError(tok, Severity::style, "unreachableCode", "Unreachable code below a 'return'"); +} + +//--------------------------------------------------------------------------- + + + + +//--------------------------------------------------------------------------- +// Usage of function variables +//--------------------------------------------------------------------------- + +static bool isOp(const Token *tok) +{ + return bool(tok && + (tok->str() == "&&" || + tok->str() == "||" || + tok->str() == "==" || + tok->str() == "!=" || + tok->str() == "<" || + tok->str() == "<=" || + tok->str() == ">" || + tok->str() == ">=" || + tok->str() == "<<" || + Token::Match(tok, "[+-*/%&!~|^,[])?:]"))); +} + +void CheckOther::functionVariableUsage() +{ + // Parse all executing scopes.. + const Token *tok1 = _tokenizer->tokens(); + if (!tok1) + return; + + while ((tok1 = Token::findmatch(tok1->next(), ") const| {")) != NULL) + { + // Varname, usage {1=declare, 2=read, 4=write} + std::map varUsage; + static const unsigned int USAGE_DECLARE = 1; + static const unsigned int USAGE_READ = 2; + static const unsigned int USAGE_WRITE = 4; + + int indentlevel = 0; + for (const Token *tok = tok1->next(); tok; tok = tok->next()) + { + if (tok->str() == "{") + ++indentlevel; + else if (tok->str() == "}") + { + --indentlevel; + if (indentlevel <= 0) + break; + } + else if (Token::Match(tok, "struct|union|class {") || + Token::Match(tok, "struct|union|class %type% {")) + { + int indentlevel0 = indentlevel; + + while (tok->str() != "{") + tok = tok->next(); + + do + { + if (tok->str() == "{") + indentlevel++; + else if (tok->str() == "}") + indentlevel--; + tok = tok->next(); + } + while (tok && indentlevel > indentlevel0); + + if (! tok) + break; + } + + if (Token::Match(tok, "[;{}] bool|char|short|int|long|float|double %var% ;|=")) + varUsage[ tok->strAt(2)] = USAGE_DECLARE; + + else if (Token::Match(tok, "[;{}] bool|char|short|int|long|float|double * %var% ;|=")) + varUsage[ tok->strAt(3)] = USAGE_DECLARE; + + else if (Token::Match(tok, "delete|return %var%")) + varUsage[ tok->strAt(1)] |= USAGE_READ; + + else if (Token::Match(tok, "%var% =")) + varUsage[ tok->str()] |= USAGE_WRITE; + + else if (Token::Match(tok, "else %var% =")) + varUsage[ tok->strAt(1)] |= USAGE_WRITE; + + else if (Token::Match(tok, ">>|& %var%")) + varUsage[ tok->strAt(1)] |= (USAGE_WRITE | USAGE_READ); + + else if ((Token::Match(tok, "[(=&!]") || isOp(tok)) && Token::Match(tok->next(), "%var%")) + varUsage[ tok->strAt(1)] |= USAGE_READ; + + else if (Token::Match(tok, "-=|+=|*=|/=|&=|^= %var%") || Token::Match(tok, "|= %var%")) + varUsage[ tok->strAt(1)] |= USAGE_READ; + + else if (Token::Match(tok, "%var%") && (tok->next()->str() == ")" || isOp(tok->next()))) + varUsage[ tok->str()] |= USAGE_READ; + + else if (Token::Match(tok, "[(,] %var% [,)]")) + varUsage[ tok->strAt(1)] |= (USAGE_WRITE | USAGE_READ); + + else if (Token::Match(tok, "; %var% ;")) + varUsage[ tok->strAt(1)] |= USAGE_READ; + } + + // Check usage of all variables in the current scope.. + for (std::map::const_iterator it = varUsage.begin(); it != varUsage.end(); ++it) + { + std::string varname = it->first; + unsigned int usage = it->second; + + if (!std::isalpha(varname[0])) + continue; + + if (!(usage & USAGE_DECLARE)) + continue; + + if (usage == USAGE_DECLARE) + { + unusedVariableError(tok1->next(), varname); + } + + else if (!(usage & USAGE_READ)) + { + unreadVariableError(tok1->next(), varname); + } + + else if (!(usage & USAGE_WRITE)) + { + unassignedVariableError(tok1->next(), varname); + } + } + } +} + +void CheckOther::unusedVariableError(const Token *tok, const std::string &varname) +{ + reportError(tok, Severity::style, "unusedVariable", "Unused variable: " + varname); +} + +void CheckOther::unreadVariableError(const Token *tok, const std::string &varname) +{ + reportError(tok, Severity::style, "unreadVariable", "Variable '" + varname + "' is assigned a value that is never used"); +} + +void CheckOther::unassignedVariableError(const Token *tok, const std::string &varname) +{ + reportError(tok, Severity::style, "unassignedVariable", "Variable '" + varname + "' is not assigned a value"); +} +//--------------------------------------------------------------------------- + + + + + //--------------------------------------------------------------------------- // Check scope of variables.. //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index b19a3a6c4..325b0d536 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -53,6 +53,7 @@ public: checkOther.warningOldStylePointerCast(); checkOther.checkUnsignedDivision(); checkOther.checkCharVariable(); + checkOther.functionVariableUsage(); checkOther.checkVariableScope(); checkOther.checkStructMemberUsage(); } @@ -67,6 +68,7 @@ public: checkOther.warningRedundantCode(); checkOther.checkConstantFunctionParameter(); checkOther.checkIncompleteStatement(); + checkOther.unreachableCode(); if (settings->_showAll) { checkOther.postIncrement(); @@ -98,6 +100,16 @@ public: // Check for unsigned division that might create bad results void checkUnsignedDivision(); + /** Check for unreachable code */ + void unreachableCode(); + void unreachableCodeError(const Token *tok); + + /** Check for unused function variables */ + void functionVariableUsage(); + void unusedVariableError(const Token *tok, const std::string &varname); + void unreadVariableError(const Token *tok, const std::string &varname); + void unassignedVariableError(const Token *tok, const std::string &varname); + // Check scope of variables void checkVariableScope(); diff --git a/test/testother.cpp b/test/testother.cpp index e8b4c2488..d2c6f164d 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -108,6 +108,7 @@ private: CheckOther checkOther(&tokenizer, &settings, this); checkOther.warningRedundantCode(); checkOther.checkZeroDivision(); + checkOther.unreachableCode(); } @@ -324,7 +325,7 @@ private: " break;\n" " }\n" "}\n"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (style) Unreachable code below a 'return'\n", errout.str()); } diff --git a/test/testunusedvar.cpp b/test/testunusedvar.cpp index de2935d7a..5624de3fb 100644 --- a/test/testunusedvar.cpp +++ b/test/testunusedvar.cpp @@ -58,6 +58,26 @@ private: TEST_CASE(structmember4); TEST_CASE(structmember5); TEST_CASE(structmember6); + + TEST_CASE(localvar1); + TEST_CASE(localvar2); + TEST_CASE(localvar3); + TEST_CASE(localvar4); + TEST_CASE(localvar5); + + // Don't give false positives for variables in structs/unions + TEST_CASE(localvarStruct1); + TEST_CASE(localvarStruct2); + TEST_CASE(localvarStruct3); + TEST_CASE(localvarStruct4); // Ticket #31: sigsegv on incomplete struct + + TEST_CASE(localvarOp); // Usage with arithmetic operators + TEST_CASE(localvarInvert); // Usage with inverted variable + TEST_CASE(localvarIf); // Usage in if + TEST_CASE(localvarIfElse); // return tmp1 ? tmp2 : tmp3; + TEST_CASE(localvarOpAssign); // a |= b; + TEST_CASE(localvarFor); // for ( ; var; ) + TEST_CASE(localvarShift); // 1 >> var } void structmember1() @@ -177,6 +197,208 @@ private: "}\n"); ASSERT_EQUALS("", errout.str()); } + + + + + + + + + + void functionVariableUsage(const char code[]) + { + // Tokenize.. + Tokenizer tokenizer; + std::istringstream istr(code); + tokenizer.tokenize(istr, "test.cpp"); + + // Clear the error buffer.. + errout.str(""); + + // Check for unused variables.. + Settings settings; + CheckOther checkOther(&tokenizer, &settings, this); + checkOther.functionVariableUsage(); + } + + void localvar1() + { + functionVariableUsage("void foo()\n" + "{\n" + " int i = 0;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2]: (style) Variable 'i' is assigned a value that is never used\n", errout.str()); + } + + void localvar2() + { + functionVariableUsage("void foo()\n" + "{\n" + " int i;\n" + " return i;\n" + "}\n"); + ASSERT_EQUALS(std::string("[test.cpp:2]: (style) Variable 'i' is not assigned a value\n"), errout.str()); + } + + void localvar3() + { + functionVariableUsage("void foo()\n" + "{\n" + " int i;\n" + " if ( abc )\n" + " ;\n" + " else i = 0;\n" + "}\n"); + ASSERT_EQUALS(std::string("[test.cpp:2]: (style) Variable 'i' is assigned a value that is never used\n"), errout.str()); + } + + void localvar4() + { + functionVariableUsage("void foo()\n" + "{\n" + " int i = 0;\n" + " f(i);\n" + "}\n"); + ASSERT_EQUALS(std::string(""), errout.str()); + } + + void localvar5() + { + functionVariableUsage("void foo()\n" + "{\n" + " int a = 0;\n" + " b = (char)a;\n" + "}\n"); + ASSERT_EQUALS(std::string(""), errout.str()); + } + + + + void localvarStruct1() + { + functionVariableUsage("void foo()\n" + "{\n" + " static const struct{ int x, y, w, h; } bounds = {1,2,3,4};\n" + " return bounds.x + bounds.y + bounds.w + bounds.h;\n" + "}\n"); + ASSERT_EQUALS(std::string(""), errout.str()); + } + + void localvarStruct2() + { + functionVariableUsage("void foo()\n" + "{\n" + " struct ABC { int a, b, c; };\n" + " struct ABC abc = { 1, 2, 3 };\n" + "}\n"); + ASSERT_EQUALS(std::string(""), errout.str()); + } + + void localvarStruct3() + { + functionVariableUsage("void foo()\n" + "{\n" + " int a = 10;\n" + " union { struct { unsigned char x; }; unsigned char z; };\n" + " do {\n" + " func();\n" + " } while(a--);\n" + "}\n"); + ASSERT_EQUALS(std::string(""), errout.str()); + } + + void localvarStruct4() + { + /* This must not SIGSEGV: */ + // FIXME!! + //functionVariableUsage("void foo()\n" + // "{\n" + // " struct { \n"); + } + + + + void localvarOp() + { + const char op[] = "+-*/%&|^"; + for (const char *p = op; *p; ++p) + { + std::string code("int main()\n" + "{\n" + " int tmp = 10;\n" + " return 123 " + std::string(1, *p) + " tmp;\n" + "}\n"); + functionVariableUsage(code.c_str()); + ASSERT_EQUALS(std::string(""), errout.str()); + } + } + + void localvarInvert() + { + functionVariableUsage("int main()\n" + "{\n" + " int tmp = 10;\n" + " return ~tmp;\n" + "}\n"); + ASSERT_EQUALS(std::string(""), errout.str()); + } + + void localvarIf() + { + functionVariableUsage("int main()\n" + "{\n" + " int tmp = 10;\n" + " if ( tmp )\n" + " return 1;\n" + " return 0;\n" + "}\n"); + ASSERT_EQUALS(std::string(""), errout.str()); + } + + void localvarIfElse() + { + functionVariableUsage("int foo()\n" + "{\n" + " int tmp1 = 1;\n" + " int tmp2 = 2;\n" + " int tmp3 = 3;\n" + " return tmp1 ? tmp2 : tmp3;\n" + "}\n"); + ASSERT_EQUALS(std::string(""), errout.str()); + } + + void localvarOpAssign() + { + functionVariableUsage("void foo()\n" + "{\n" + " int a = 1;\n" + " int b = 2;\n" + " a |= b;\n" + "}\n"); + ASSERT_EQUALS(std::string("[test.cpp:2]: (style) Variable 'a' is assigned a value that is never used\n"), errout.str()); + } + + void localvarFor() + { + functionVariableUsage("void foo()\n" + "{\n" + " int a = 1;\n" + " for (;a;);\n" + "}\n"); + ASSERT_EQUALS(std::string(""), errout.str()); + } + + void localvarShift() + { + functionVariableUsage("int foo()\n" + "{\n" + " int var = 1;\n" + " return 1 >> var;\n" + "}\n"); + ASSERT_EQUALS(std::string(""), errout.str()); + } + }; REGISTER_TEST(TestUnusedVar)