From 452ecc7ceb0ed522f281cf4da4871f5f732cd6e3 Mon Sep 17 00:00:00 2001 From: PKEuS Date: Thu, 28 Jul 2016 13:19:24 +0200 Subject: [PATCH] Improved check: Detect passedByValue even for arguments that are not declared as "const" --- lib/checkother.cpp | 121 ++++++++++++++++++++++++++++++++++++++------ lib/checkother.h | 12 ++--- test/testother.cpp | 123 +++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 233 insertions(+), 23 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 0b295163b..0d2fa2674 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -1383,9 +1383,39 @@ void CheckOther::commaSeparatedReturnError(const Token *tok) } //--------------------------------------------------------------------------- -// Check for constant function parameters +// Check for function parameters that should be passed by reference //--------------------------------------------------------------------------- -void CheckOther::checkConstantFunctionParameter() +static std::size_t estimateSize(const Type* type, const Settings* settings, const SymbolDatabase* symbolDatabase, std::size_t recursionDepth = 0) +{ + if (recursionDepth > 20) + return 0; + + std::size_t cumulatedSize = 0; + for (std::list::const_iterator i = type->classScope->varlist.cbegin(); i != type->classScope->varlist.cend(); ++i) { + std::size_t size = 0; + if (i->isStatic()) + continue; + if (i->isPointer() || i->isReference()) + size = settings->sizeof_pointer; + else if (i->type() && i->type()->classScope) + size = estimateSize(i->type(), settings, symbolDatabase, recursionDepth+1); + else if (i->isStlStringType() || (i->isStlType() && Token::Match(i->typeStartToken(), "std :: %type% <") && !Token::simpleMatch(i->typeStartToken()->linkAt(3), "> ::"))) + size = 16; // Just guess + else + size = symbolDatabase->sizeOfType(i->typeStartToken()); + + if (i->isArray()) + cumulatedSize += size*i->dimension(0); + else + cumulatedSize += size; + } + for (std::vector::const_iterator i = type->derivedFrom.cbegin(); i != type->derivedFrom.cend(); ++i) + if (i->type && i->type->classScope) + cumulatedSize += estimateSize(i->type, settings, symbolDatabase, recursionDepth+1); + return cumulatedSize; +} + +void CheckOther::checkPassByReference() { if (!_settings->isEnabled("performance") || _tokenizer->isC()) return; @@ -1394,34 +1424,97 @@ void CheckOther::checkConstantFunctionParameter() for (unsigned int i = 1; i < symbolDatabase->getVariableListSize(); i++) { const Variable* var = symbolDatabase->getVariableFromVarId(i); - if (!var || !var->isArgument() || !var->isClass() || !var->isConst() || var->isPointer() || var->isArray() || var->isReference() || var->isEnumType()) + if (!var || !var->isArgument() || !var->isClass() || var->isPointer() || var->isArray() || var->isReference() || var->isEnumType()) continue; if (var->scope() && var->scope()->function->arg->link()->strAt(-1) == ".") continue; // references could not be used as va_start parameters (#5824) + bool inconclusive = false; + const Token* const tok = var->typeStartToken(); - // TODO: False negatives. This pattern only checks for string. - // Investigate if there are other classes in the std - // namespace and add them to the pattern. There are - // streams for example (however it seems strange with - // const stream parameter). if (var->isStlStringType()) { - passedByValueError(tok, var->name()); + ; } else if (var->isStlType() && Token::Match(tok, "std :: %type% <") && !Token::simpleMatch(tok->linkAt(3), "> ::")) { - passedByValueError(tok, var->name()); - } else if (var->type()) { // Check if type is a struct or class. - passedByValueError(tok, var->name()); + ; + } else if (var->type() && !var->type()->isEnumType()) { // Check if type is a struct or class. + // Ensure that it is a large object. + if (!var->type()->classScope) + inconclusive = true; + else if (estimateSize(var->type(), _settings, symbolDatabase) <= 8) + continue; + } else + continue; + + if (inconclusive && !_settings->inconclusive) + 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 (tok->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 (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); } } -void CheckOther::passedByValueError(const Token *tok, const std::string &parname) +void CheckOther::passedByValueError(const Token *tok, const std::string &parname, bool inconclusive) { reportError(tok, Severity::performance, "passedByValue", "Function parameter '" + parname + "' should be passed by reference.\n" "Parameter '" + parname + "' is passed by value. It could be passed " - "as a (const) reference which is usually faster and recommended in C++.", CWE398, false); + "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 8a5265b06..87f480db4 100644 --- a/lib/checkother.h +++ b/lib/checkother.h @@ -81,7 +81,7 @@ public: // Checks checkOther.clarifyCalculation(); checkOther.clarifyStatement(); - checkOther.checkConstantFunctionParameter(); + checkOther.checkPassByReference(); checkOther.checkIncompleteStatement(); checkOther.checkCastIntToCharAndBack(); @@ -115,8 +115,8 @@ public: /** @brief %Check for comma separated statements in return */ void checkCommaSeparatedReturn(); - /** @brief %Check for constant function parameter */ - void checkConstantFunctionParameter(); + /** @brief %Check for function parameters that should be passed by reference */ + void checkPassByReference(); /** @brief Using char variable as array index / as operand in bit operation */ void checkCharVariable(); @@ -212,7 +212,7 @@ private: void clarifyStatementError(const Token* tok); void cstyleCastError(const Token *tok); void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to, bool inconclusive); - void passedByValueError(const Token *tok, const std::string &parname); + void passedByValueError(const Token *tok, const std::string &parname, bool inconclusive); void constStatementError(const Token *tok, const std::string &type); void signedCharArrayIndexError(const Token *tok); void unknownSignCharArrayIndexError(const Token *tok); @@ -274,7 +274,7 @@ private: c.checkComparisonFunctionIsAlwaysTrueOrFalseError(nullptr, "isless","varName",false); c.checkCastIntToCharAndBackError(nullptr, "func_name"); c.cstyleCastError(nullptr); - c.passedByValueError(nullptr, "parametername"); + c.passedByValueError(nullptr, "parametername", false); c.constStatementError(nullptr, "type"); c.signedCharArrayIndexError(nullptr); c.unknownSignCharArrayIndexError(nullptr); @@ -335,6 +335,7 @@ private: // performance "- redundant data copying for const variable\n" "- subsequent assignment or copying to a variable or buffer\n" + "- passing parameter by value\n" // portability "- memset() with a float as the 2nd parameter\n" @@ -343,7 +344,6 @@ private: // style "- C-style pointer cast in C++ code\n" "- casting between incompatible pointer types\n" - "- passing parameter by value\n" "- [Incomplete statement](IncompleteStatement)\n" "- [check how signed char variables are used](CharVar)\n" "- variable scope can be limited\n" diff --git a/test/testother.cpp b/test/testother.cpp index b194ff7b7..0a2be3acd 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -82,6 +82,7 @@ private: TEST_CASE(invalidPointerCast); TEST_CASE(passedByValue); + TEST_CASE(passedByValue_nonConst); TEST_CASE(switchRedundantAssignmentTest); TEST_CASE(switchRedundantOperationTest); @@ -1292,7 +1293,28 @@ private: check("void f(const std::string::size_type x) {}"); ASSERT_EQUALS("", errout.str()); - check("class Foo;\nvoid f(const Foo foo) {}"); + 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()); + + 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()); + + 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()); + + check("class Foo { std::string* s; };\nvoid f(const Foo foo) {}"); // Small class (pointer) + ASSERT_EQUALS("", errout.str()); + + check("class Foo { static std::string s; };\nvoid f(const Foo foo) {}"); // Small class (static member) + ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by reference.\n", 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()); + + 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()); check("void f(const std::string &str) {}"); @@ -1341,10 +1363,105 @@ private: "}; "); ASSERT_EQUALS("", errout.str()); + 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()); + } + + 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()); + + 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()); + + 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()); + + check("void f(std::string str) {\n" + " std::cin >> str;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void f(std::string str) {\n" + " foo(str);\n" // It could be that foo takes str as non-const-reference + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(const 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()); + + 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()); + + check("void foo(std::string& str);\n" + "void f(std::string str) {\n" + " foo(str);\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("void foo(int& i1, const std::string& str, int& i2);\n" + "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()); + + check("std::string f(std::string str) {\n" + " str += x;\n" + " return str;\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + check("class X {\n" + " std::string s;\n" + " void func() const;\n" + "};\n" + "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()); + + check("class X {\n" + " void func();\n" + "};\n" + "Y f(X x) {\n" + " x.func();\n" + "}"); + ASSERT_EQUALS("", errout.str()); + + 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()); + + check("class X {\n" + " virtual void func(std::string str) {}\n" // Do not warn about virtual functions, if 'str' is not declared as const + "};"); + ASSERT_EQUALS("", errout.str()); + + check("class X {\n" + " uint64_t i;\n" + "};\n" + "class Y : X {\n" + " uint64_t j;\n" + "};\n" + "void f(X x, Y y) {\n" + "}"); + ASSERT_EQUALS("[test.cpp:7]: (performance) Function parameter 'y' should be passed by reference.\n", errout.str()); } void switchRedundantAssignmentTest() { - check("void foo()\n" "{\n" " int y = 1;\n" @@ -3436,7 +3553,7 @@ private: "}"); ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '|'.\n", errout.str()); - check("void foo(std::string a, std::string b) {\n" + check("void foo(const std::string& a, const std::string& b) {\n" " return a.find(b+\"&\") || a.find(\"&\"+b);\n" "}"); ASSERT_EQUALS("", errout.str());