From d5fb529d4f4d97e2959c6b94cf43f24b1be30cef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= Date: Fri, 20 Apr 2018 17:33:42 +0200 Subject: [PATCH] Fixed #8492 (Improve message: parameter should be passed by reference) --- lib/checkother.cpp | 133 +++++++++++++++++++++++---------------------- lib/checkother.h | 2 +- test/testother.cpp | 54 +++++++++--------- 3 files changed, 97 insertions(+), 92 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index dde9c0bbd..633ab6e73 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -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) { @@ -1352,6 +1352,63 @@ static std::size_t estimateSize(const Type* type, const Settings* settings, cons 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() { if (!_settings->isEnabled(Settings::PERFORMANCE) || _tokenizer->isC()) @@ -1387,70 +1444,18 @@ void CheckOther::checkPassByReference() continue; bool isConst = var->isConst(); - if (!isConst) { - // Check if variable could be const - if (!var->scope() || var->scope()->function->isVirtual()) - 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) { + passedByValueError(tok, var->name(), inconclusive); + continue; } - if (isConst) + // Check if variable could be const + if (!var->scope() || var->scope()->function->isVirtual()) + continue; + + if (canBeConst(var)) { passedByValueError(tok, var->name(), inconclusive); + } } } @@ -1458,9 +1463,9 @@ void CheckOther::passedByValueError(const Token *tok, const std::string &parname { reportError(tok, Severity::performance, "passedByValue", "$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 " - "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); } //--------------------------------------------------------------------------- diff --git a/lib/checkother.h b/lib/checkother.h index fec815eeb..967d148c2 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -284,7 +284,7 @@ private: c.checkComparisonFunctionIsAlwaysTrueOrFalseError(nullptr, "isless","varName",false); c.checkCastIntToCharAndBackError(nullptr, "func_name"); c.cstyleCastError(nullptr); - c.passedByValueError(nullptr, "parametername", false); + c.passedByValueError(nullptr, "parametername", false); c.constStatementError(nullptr, "type"); c.signedCharArrayIndexError(nullptr); c.unknownSignCharArrayIndexError(nullptr); diff --git a/test/testother.cpp b/test/testother.cpp index bcbf33977..19eae5f86 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -1371,7 +1371,7 @@ private: void passedByValue() { 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 ptr) {}"); ASSERT_EQUALS("", errout.str()); @@ -1380,16 +1380,16 @@ private: ASSERT_EQUALS("", errout.str()); 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 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 ASSERT_EQUALS("", errout.str()); 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) ASSERT_EQUALS("", errout.str()); @@ -1398,19 +1398,19 @@ private: ASSERT_EQUALS("", errout.str()); 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) - 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) {}"); ASSERT_EQUALS("", errout.str()); check("void f(const std::vector 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 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::size_type s) {}"); ASSERT_EQUALS("", errout.str()); @@ -1422,16 +1422,16 @@ private: ASSERT_EQUALS("", errout.str()); check("void f(const std::map 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 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 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 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) {}"); ASSERT_EQUALS("", errout.str()); @@ -1455,22 +1455,22 @@ private: check("class X {\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() { 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" " 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" " 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" " std::cin >> str;\n" @@ -1480,7 +1480,7 @@ private: check("void f(std::string 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" " std::string& s2 = str;\n" @@ -1490,7 +1490,7 @@ private: check("void f(std::string 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" " str = \"\";\n" @@ -1506,13 +1506,13 @@ private: "void f(std::string 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" "void f(std::string 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" "void f(std::string str) {\n" @@ -1524,7 +1524,7 @@ private: "void f(std::string str) {\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" " str += x;\n" @@ -1539,7 +1539,7 @@ private: "Y f(X x) {\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" " void func();\n" @@ -1552,7 +1552,7 @@ private: check("class X {\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" " 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" "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" " void* a;\n" @@ -1580,10 +1580,10 @@ private: "};\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" " uint64_t a;\n" " uint64_t b;\n" @@ -1593,7 +1593,7 @@ private: Settings s32(_settings); s32.platform(cppcheck::Platform::Unix32); 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); s64.platform(cppcheck::Platform::Unix64);