Permalink
Browse files

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
  • Loading branch information...
1 parent 1c58420 commit 33cf561d85f5d66ffd5516b9fd628602aca37669 @PKEuS PKEuS committed Feb 18, 2013
Showing with 62 additions and 14 deletions.
  1. +18 −8 lib/checkautovariables.cpp
  2. +5 −4 lib/checkautovariables.h
  3. +39 −2 test/testautovariables.cpp
View
26 lib/checkautovariables.cpp
@@ -36,18 +36,25 @@ 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();
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)
View
9 lib/checkautovariables.h
@@ -63,10 +63,11 @@ class CPPCHECKLIB CheckAutoVariables : public Check {
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?
View
41 test/testautovariables.cpp
@@ -112,7 +112,7 @@ class TestAutoVariables : public TestFixture {
" 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 @@ class TestAutoVariables : public TestFixture {
" 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 @@ class TestAutoVariables : public TestFixture {
"}");
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 @@ class TestAutoVariables : public TestFixture {
" 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() {

0 comments on commit 33cf561

Please sign in to comment.