walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Fixed #8492 (Improve message: parameter should be passed by reference)

This commit is contained in:
Daniel Marjamäki 2018-04-20 17:33:42 +02:00
parent 86a9deffbf
commit d5fb529d4f
3 changed files with 97 additions and 92 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

133
lib/checkother.cpp
View File

@ -1320,7 +1320,7 @@ void CheckOther::commaSeparatedReturnError(const Token *tok)
} }
//--------------------------------------------------------------------------- //---------------------------------------------------------------------------
// Check for function parameters that should be passed by reference // Check for function parameters that should be passed by const reference
//--------------------------------------------------------------------------- //---------------------------------------------------------------------------
static std::size_t estimateSize(const Type* type, const Settings* settings, const SymbolDatabase* symbolDatabase, std::size_t recursionDepth = 0) static std::size_t estimateSize(const Type* type, const Settings* settings, const SymbolDatabase* symbolDatabase, std::size_t recursionDepth = 0)
{ {
@ -1352,6 +1352,63 @@ static std::size_t estimateSize(const Type* type, const Settings* settings, cons
return cumulatedSize; return cumulatedSize;
} }
static bool canBeConst(const Variable *var)
{
for (const Token* tok2 = var->scope()->classStart; tok2 != var->scope()->classEnd; tok2 = tok2->next()) {
if (tok2->varId() == var->declarationId()) {
const Token* parent = tok2->astParent();
if (!parent)
;
else if (parent->str() == "<<" || parent->str() == ">>") {
if (parent->str() == "<<" && parent->astOperand1() == tok2)
return false;
else if (parent->str() == ">>" && parent->astOperand2() == tok2)
return false;
} else if (parent->str() == "," || parent->str() == "(") { // function argument
const Token* tok3 = tok2->previous();
unsigned int argNr = 0;
while (tok3 && tok3->str() != "(") {
if (tok3->link() && Token::Match(tok3, ")|]|}|>"))
tok3 = tok3->link();
else if (tok3->link())
break;
else if (tok3->str() == ";")
break;
else if (tok3->str() == ",")
argNr++;
tok3 = tok3->previous();
}
if (!tok3 || tok3->str() != "(" || !tok3->astOperand1() || !tok3->astOperand1()->function())
return false;
else {
const Variable* argVar = tok3->astOperand1()->function()->getArgumentVar(argNr);
if (!argVar|| (!argVar->isConst() && argVar->isReference()))
return false;
}
} else if (parent->isConstOp())
;
else if (parent->isAssignmentOp()) {
if (parent->astOperand1() == tok2)
return false;
else if (parent->astOperand1()->str() == "&") {
const Variable* assignedVar = parent->previous()->variable();
if (!assignedVar || !assignedVar->isConst())
return false;
}
} else if (Token::Match(tok2, "%var% . %name% (")) {
const Function* func = tok2->tokAt(2)->function();
if (func && (func->isConst() || func->isStatic()))
;
else
return false;
} else
return false;
}
}
return true;
}
void CheckOther::checkPassByReference() void CheckOther::checkPassByReference()
{ {
if (!_settings->isEnabled(Settings::PERFORMANCE) || _tokenizer->isC()) if (!_settings->isEnabled(Settings::PERFORMANCE) || _tokenizer->isC())
@ -1387,70 +1444,18 @@ void CheckOther::checkPassByReference()
continue; continue;
bool isConst = var->isConst(); bool isConst = var->isConst();
if (!isConst) { if (isConst) {
// Check if variable could be const passedByValueError(tok, var->name(), inconclusive);
if (!var->scope() || var->scope()->function->isVirtual()) continue;
continue;
isConst = true;
for (const Token* tok2 = var->scope()->classStart; tok2 != var->scope()->classEnd; tok2 = tok2->next()) {
if (tok2->varId() == var->declarationId()) {
const Token* parent = tok2->astParent();
if (!parent)
;
else if (parent->str() == "<<" || parent->str() == ">>") {
if (parent->str() == "<<" && parent->astOperand1() == tok2)
isConst = false;
else if (parent->str() == ">>" && parent->astOperand2() == tok2)
isConst = false;
} else if (parent->str() == "," || parent->str() == "(") { // function argument
const Token* tok3 = tok2->previous();
unsigned int argNr = 0;
while (tok3 && tok3->str() != "(") {
if (tok3->link() && Token::Match(tok3, ")|]|}|>"))
tok3 = tok3->link();
else if (tok3->link())
break;
else if (tok3->str() == ";")
break;
else if (tok3->str() == ",")
argNr++;
tok3 = tok3->previous();
}
if (!tok3 || tok3->str() != "(" || !tok3->astOperand1() || !tok3->astOperand1()->function())
isConst = false;
else {
const Variable* argVar = tok3->astOperand1()->function()->getArgumentVar(argNr);
if (!argVar|| (!argVar->isConst() && argVar->isReference()))
isConst = false;
}
} else if (parent->isConstOp())
;
else if (parent->isAssignmentOp()) {
if (parent->astOperand1() == tok2)
isConst = false;
else if (parent->astOperand1()->str() == "&") {
const Variable* assignedVar = parent->previous()->variable();
if (!assignedVar || !assignedVar->isConst())
isConst = false;
}
} else if (Token::Match(tok2, "%var% . %name% (")) {
const Function* func = tok2->tokAt(2)->function();
if (func && (func->isConst() || func->isStatic()))
;
else
isConst = false;
} else
isConst = false;
if (!isConst)
break;
}
}
} }
if (isConst) // Check if variable could be const
if (!var->scope() || var->scope()->function->isVirtual())
continue;
if (canBeConst(var)) {
passedByValueError(tok, var->name(), inconclusive); passedByValueError(tok, var->name(), inconclusive);
}
} }
} }
@ -1458,9 +1463,9 @@ void CheckOther::passedByValueError(const Token *tok, const std::string &parname
{ {
reportError(tok, Severity::performance, "passedByValue", reportError(tok, Severity::performance, "passedByValue",
"$symbol:" + parname + "\n" "$symbol:" + parname + "\n"
"Function parameter '$symbol' should be passed by reference.\n" "Function parameter '$symbol' should be passed by const reference.\n"
"Parameter '$symbol' is passed by value. It could be passed " "Parameter '$symbol' is passed by value. It could be passed "
"as a (const) reference which is usually faster and recommended in C++.", CWE398, inconclusive); "as a const reference which is usually faster and recommended in C++.", CWE398, inconclusive);
} }
//--------------------------------------------------------------------------- //---------------------------------------------------------------------------

2
lib/checkother.h
View File

@ -284,7 +284,7 @@ private:
c.checkComparisonFunctionIsAlwaysTrueOrFalseError(nullptr, "isless","varName",false); c.checkComparisonFunctionIsAlwaysTrueOrFalseError(nullptr, "isless","varName",false);
c.checkCastIntToCharAndBackError(nullptr, "func_name"); c.checkCastIntToCharAndBackError(nullptr, "func_name");
c.cstyleCastError(nullptr); c.cstyleCastError(nullptr);
c.passedByValueError(nullptr, "parametername", false); c.passedByValueError(nullptr, "parametername", false);
c.constStatementError(nullptr, "type"); c.constStatementError(nullptr, "type");
c.signedCharArrayIndexError(nullptr); c.signedCharArrayIndexError(nullptr);
c.unknownSignCharArrayIndexError(nullptr); c.unknownSignCharArrayIndexError(nullptr);

54
test/testother.cpp
View File

@ -1371,7 +1371,7 @@ private:
void passedByValue() { void passedByValue() {
check("void f(const std::string str) {}"); check("void f(const std::string str) {}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by const reference.\n", errout.str());
check("void f(std::unique_ptr<std::string> ptr) {}"); check("void f(std::unique_ptr<std::string> ptr) {}");
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
@ -1380,16 +1380,16 @@ private:
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
check("class Foo;\nvoid f(const Foo foo) {}"); // Unknown class check("class Foo;\nvoid f(const Foo foo) {}"); // Unknown class
ASSERT_EQUALS("[test.cpp:2]: (performance, inconclusive) Function parameter 'foo' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (performance, inconclusive) Function parameter 'foo' should be passed by const reference.\n", errout.str());
check("class Foo { std::vector<int> v; };\nvoid f(const Foo foo) {}"); // Large class (STL member) check("class Foo { std::vector<int> v; };\nvoid f(const Foo foo) {}"); // Large class (STL member)
ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by const reference.\n", errout.str());
check("class Foo { int i; };\nvoid f(const Foo foo) {}"); // Small class check("class Foo { int i; };\nvoid f(const Foo foo) {}"); // Small class
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
check("class Foo { int i[6]; };\nvoid f(const Foo foo) {}"); // Large class (array) check("class Foo { int i[6]; };\nvoid f(const Foo foo) {}"); // Large class (array)
ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by const reference.\n", errout.str());
check("class Foo { std::string* s; };\nvoid f(const Foo foo) {}"); // Small class (pointer) check("class Foo { std::string* s; };\nvoid f(const Foo foo) {}"); // Small class (pointer)
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
@ -1398,19 +1398,19 @@ private:
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
check("class X { std::string s; }; class Foo : X { };\nvoid f(const Foo foo) {}"); // Large class (inherited) check("class X { std::string s; }; class Foo : X { };\nvoid f(const Foo foo) {}"); // Large class (inherited)
ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by const reference.\n", errout.str());
check("class X { std::string s; }; class Foo { X x; };\nvoid f(const Foo foo) {}"); // Large class (inherited) check("class X { std::string s; }; class Foo { X x; };\nvoid f(const Foo foo) {}"); // Large class (inherited)
ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by const reference.\n", errout.str());
check("void f(const std::string &str) {}"); check("void f(const std::string &str) {}");
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
check("void f(const std::vector<int> v) {}"); check("void f(const std::vector<int> v) {}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by const reference.\n", errout.str());
check("void f(const std::vector<std::string> v) {}"); check("void f(const std::vector<std::string> v) {}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by const reference.\n", errout.str());
check("void f(const std::vector<std::string>::size_type s) {}"); check("void f(const std::vector<std::string>::size_type s) {}");
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
@ -1422,16 +1422,16 @@ private:
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
check("void f(const std::map<int,int> v) {}"); check("void f(const std::map<int,int> v) {}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by const reference.\n", errout.str());
check("void f(const std::map<std::string,std::string> v) {}"); check("void f(const std::map<std::string,std::string> v) {}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by const reference.\n", errout.str());
check("void f(const std::map<int,std::string> v) {}"); check("void f(const std::map<int,std::string> v) {}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by const reference.\n", errout.str());
check("void f(const std::map<std::string,int> v) {}"); check("void f(const std::map<std::string,int> v) {}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'v' should be passed by const reference.\n", errout.str());
check("void f(const std::streamoff pos) {}"); check("void f(const std::streamoff pos) {}");
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
@ -1455,22 +1455,22 @@ private:
check("class X {\n" check("class X {\n"
" virtual void func(const std::string str) {}\n" " virtual void func(const std::string str) {}\n"
"};"); "};");
ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by const reference.\n", errout.str());
} }
void passedByValue_nonConst() { void passedByValue_nonConst() {
check("void f(std::string str) {}"); check("void f(std::string str) {}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by const reference.\n", errout.str());
check("void f(std::string str) {\n" check("void f(std::string str) {\n"
" return str + x;\n" " return str + x;\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by const reference.\n", errout.str());
check("void f(std::string str) {\n" check("void f(std::string str) {\n"
" std::cout << str;\n" " std::cout << str;\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by const reference.\n", errout.str());
check("void f(std::string str) {\n" check("void f(std::string str) {\n"
" std::cin >> str;\n" " std::cin >> str;\n"
@ -1480,7 +1480,7 @@ private:
check("void f(std::string str) {\n" check("void f(std::string str) {\n"
" std::string s2 = str;\n" " std::string s2 = str;\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by const reference.\n", errout.str());
check("void f(std::string str) {\n" check("void f(std::string str) {\n"
" std::string& s2 = str;\n" " std::string& s2 = str;\n"
@ -1490,7 +1490,7 @@ private:
check("void f(std::string str) {\n" check("void f(std::string str) {\n"
" const std::string& s2 = str;\n" " const std::string& s2 = str;\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by const reference.\n", errout.str());
check("void f(std::string str) {\n" check("void f(std::string str) {\n"
" str = \"\";\n" " str = \"\";\n"
@ -1506,13 +1506,13 @@ private:
"void f(std::string str) {\n" "void f(std::string str) {\n"
" foo(str);\n" " foo(str);\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by const reference.\n", errout.str());
check("void foo(std::string str);\n" check("void foo(std::string str);\n"
"void f(std::string str) {\n" "void f(std::string str) {\n"
" foo(str);\n" " foo(str);\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by const reference.\n", errout.str());
check("void foo(std::string& str);\n" check("void foo(std::string& str);\n"
"void f(std::string str) {\n" "void f(std::string str) {\n"
@ -1524,7 +1524,7 @@ private:
"void f(std::string str) {\n" "void f(std::string str) {\n"
" foo((a+b)*c, str, x);\n" " foo((a+b)*c, str, x);\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by const reference.\n", errout.str());
check("std::string f(std::string str) {\n" check("std::string f(std::string str) {\n"
" str += x;\n" " str += x;\n"
@ -1539,7 +1539,7 @@ private:
"Y f(X x) {\n" "Y f(X x) {\n"
" x.func();\n" " x.func();\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:5]: (performance) Function parameter 'x' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:5]: (performance) Function parameter 'x' should be passed by const reference.\n", errout.str());
check("class X {\n" check("class X {\n"
" void func();\n" " void func();\n"
@ -1552,7 +1552,7 @@ private:
check("class X {\n" check("class X {\n"
" void func(std::string str) {}\n" " void func(std::string str) {}\n"
"};"); "};");
ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by const reference.\n", errout.str());
check("class X {\n" check("class X {\n"
" virtual void func(std::string str) {}\n" // Do not warn about virtual functions, if 'str' is not declared as const " virtual void func(std::string str) {}\n" // Do not warn about virtual functions, if 'str' is not declared as const
@ -1567,7 +1567,7 @@ private:
"};\n" "};\n"
"void f(Y y) {\n" "void f(Y y) {\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:7]: (performance) Function parameter 'y' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:7]: (performance) Function parameter 'y' should be passed by const reference.\n", errout.str());
check("class X {\n" check("class X {\n"
" void* a;\n" " void* a;\n"
@ -1580,10 +1580,10 @@ private:
"};\n" "};\n"
"void f(X x, Y y) {\n" "void f(X x, Y y) {\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:10]: (performance) Function parameter 'y' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:10]: (performance) Function parameter 'y' should be passed by const reference.\n", errout.str());
{ {
// 8-byte data should be passed by reference on 32-bit platform but not on 64-bit platform // 8-byte data should be passed by const reference on 32-bit platform but not on 64-bit platform
const char code[] = "class X {\n" const char code[] = "class X {\n"
" uint64_t a;\n" " uint64_t a;\n"
" uint64_t b;\n" " uint64_t b;\n"
@ -1593,7 +1593,7 @@ private:
Settings s32(_settings); Settings s32(_settings);
s32.platform(cppcheck::Platform::Unix32); s32.platform(cppcheck::Platform::Unix32);
check(code, &s32); check(code, &s32);
ASSERT_EQUALS("[test.cpp:5]: (performance) Function parameter 'x' should be passed by reference.\n", errout.str()); ASSERT_EQUALS("[test.cpp:5]: (performance) Function parameter 'x' should be passed by const reference.\n", errout.str());
Settings s64(_settings); Settings s64(_settings);
s64.platform(cppcheck::Platform::Unix64); s64.platform(cppcheck::Platform::Unix64);