From 33cf561d85f5d66ffd5516b9fd628602aca37669 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Mon, 18 Feb 2013 08:52:49 -0800 Subject: [PATCH] Refactorized check for assigning function parameters: - Fixed false negative: Check is also valid for all non-references, not only for pointers. - Fixed false negative: Usage before assignment doesn't require bailout - Fixed false positive #4598 caused by inadequate usage of CheckUninitVar::isVariableUsage - Made several member functions static --- lib/checkautovariables.cpp | 26 ++++++++++++++++-------- lib/checkautovariables.h | 9 +++++---- test/testautovariables.cpp | 41 ++++++++++++++++++++++++++++++++++++-- 3 files changed, 62 insertions(+), 14 deletions(-) diff --git a/lib/checkautovariables.cpp b/lib/checkautovariables.cpp index 70a1658fe..cc8ed4d27 100644 --- a/lib/checkautovariables.cpp +++ b/lib/checkautovariables.cpp @@ -36,6 +36,13 @@ namespace { } +bool CheckAutoVariables::isPtrArg(const Token *tok) +{ + const Variable *var = tok->variable(); + + return(var && var->isArgument() && var->isPointer()); +} + bool CheckAutoVariables::isRefPtrArg(const Token *tok) { const Variable *var = tok->variable(); @@ -43,11 +50,11 @@ bool CheckAutoVariables::isRefPtrArg(const Token *tok) return(var && var->isArgument() && var->isReference() && var->isPointer()); } -bool CheckAutoVariables::isPtrArg(const Token *tok) +bool CheckAutoVariables::isNonReferenceArg(const Token *tok) { const Variable *var = tok->variable(); - return(var && var->isArgument() && var->isPointer()); + return(var && var->isArgument() && !var->isReference() && (var->isPointer() || var->typeStartToken()->isStandardType() || var->type())); } bool CheckAutoVariables::isAutoVar(const Token *tok) @@ -80,10 +87,14 @@ static bool checkRvalueExpression(const Variable* var, const Token* next) return((next->str() != "." || (!var->isPointer() && (!var->isClass() || var->type()))) && next->strAt(2) != "."); } -static bool pointerIsDereferencedInScope(const Variable *var, const Scope *scope, const bool cpp) +static bool variableIsUsedInScope(const Token* start, unsigned int varId, const Scope *scope) { - for (const Token *tok = scope->classStart; tok && tok != scope->classEnd; tok = tok->next()) { - if (tok->varId() == var->varId() && CheckUninitVar::isVariableUsage(tok, true, cpp)) + for (const Token *tok = start; tok != scope->classEnd; tok = tok->next()) { + if (tok->varId() == varId) + return true; + if (tok->scope()->type == Scope::eFor || tok->scope()->type == Scope::eDo || tok->scope()->type == Scope::eWhile) // In case of loops, better checking would be necessary + return true; + if (Token::simpleMatch(tok, "asm (")) return true; } return false; @@ -110,9 +121,8 @@ void CheckAutoVariables::autoVariables() errorAutoVariableAssignment(tok->next(), false); } else if (reportWarnings && Token::Match(tok, "[;{}] %var% =") && - isPtrArg(tok->next()) && - Token::Match(tok->next()->variable()->typeStartToken(), "struct| %type% * %var% [,)]") && - !pointerIsDereferencedInScope(tok->next()->variable(), scope, _tokenizer->isCPP())) { + isNonReferenceArg(tok->next()) && + !variableIsUsedInScope(Token::findsimplematch(tok->tokAt(2), ";"), tok->next()->varId(), scope)) { errorUselessAssignmentPtrArg(tok->next()); } else if (Token::Match(tok, "[;{}] %var% . %var% = & %var%")) { // TODO: check if the parameter is only changed temporarily (#2969) diff --git a/lib/checkautovariables.h b/lib/checkautovariables.h index 4239f0468..04ae1de8b 100644 --- a/lib/checkautovariables.h +++ b/lib/checkautovariables.h @@ -63,10 +63,11 @@ public: void returnReference(); private: - bool isRefPtrArg(const Token *tok); - bool isPtrArg(const Token *tok); - bool isAutoVar(const Token *tok); - bool isAutoVarArray(const Token *tok); + static bool isPtrArg(const Token *tok); + static bool isRefPtrArg(const Token *tok); + static bool isNonReferenceArg(const Token *tok); + static bool isAutoVar(const Token *tok); + static bool isAutoVarArray(const Token *tok); /** * Returning a temporary object? diff --git a/test/testautovariables.cpp b/test/testautovariables.cpp index ce273dd63..585144d51 100644 --- a/test/testautovariables.cpp +++ b/test/testautovariables.cpp @@ -112,7 +112,7 @@ private: " int num = 2;\n" " res = #\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:4]: (warning) Assignment of function parameter has no effect outside the function.\n", errout.str()); check("void func1(int **res)\n" "{\n" @@ -141,7 +141,7 @@ private: " int num = 2;\n" " res = #\n" "}"); - ASSERT_EQUALS("", errout.str()); + ASSERT_EQUALS("[test.cpp:7]: (warning) Assignment of function parameter has no effect outside the function.\n", errout.str()); check("class Fred {\n" " void func1(int **res);\n" @@ -247,6 +247,11 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2]: (warning) Assignment of function parameter has no effect outside the function.\n", errout.str()); + check("void foo(int b) {\n" + " b = foo(b);\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Assignment of function parameter has no effect outside the function.\n", errout.str()); + check("void foo(char* p) {\n" " if (!p) p = buf;\n" " *p = 0;\n" @@ -258,6 +263,38 @@ private: " do_something(p);\n" "}"); ASSERT_EQUALS("", errout.str()); + + check("void foo(char* p) {\n" + " while (!p) p = buf;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(char* p) {\n" + " p = 0;\n" + " asm(\"somecmd\");\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(Foo* p) {\n" + " p = 0;\n" + "}"); + ASSERT_EQUALS("[test.cpp:2]: (warning) Assignment of function parameter has no effect outside the function.\n", errout.str()); + + check("class Foo {};\n" + "void foo(Foo p) {\n" + " p = 0;\n" + "}"); + ASSERT_EQUALS("[test.cpp:3]: (warning) Assignment of function parameter has no effect outside the function.\n", errout.str()); + + check("void foo(Foo p) {\n" + " p = 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(int& p) {\n" + " p = 0;\n" + "}"); + ASSERT_EQUALS("", errout.str()); } void testautovar_array1() {