diff --git a/lib/checkother.cpp b/lib/checkother.cpp index cf5ecb65b91..ee7e643cf01 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3290,10 +3290,63 @@ static bool constructorTakesReference(const Scope * const classScope) }); } +static bool isLargeObject(const Variable* var, const Settings& settings) +{ + return var && !var->isGlobal() && + (!var->type() || !var->type()->classScope || + (var->valueType() && var->valueType()->getSizeOf(settings, ValueType::Accuracy::LowerBound, ValueType::SizeOf::Pointer) > 2 * settings.platform.sizeof_pointer)); +} + //--------------------------------------------------------------------------- // This check rule works for checking the "const A a = getA()" usage when getA() returns "const A &" or "A &". // In most scenarios, "const A & a = getA()" will be more efficient. //--------------------------------------------------------------------------- +static bool checkFunctionReturnsRef(const Token* tok, const Settings& settings) +{ + if (!Token::Match(tok->previous(), "%name% (")) + return false; + if (!Token::Match(tok->link(), ") )|}| ;")) // bailout for usage like "const A a = getA()+3" + return false; + const Token* dot = tok->astOperand1(); + if (Token::simpleMatch(dot, ".")) { + const Token* varTok = dot->astOperand1(); + const int indirect = varTok->valueType() ? varTok->valueType()->pointer : 0; + if (isVariableChanged(tok, tok->scope()->bodyEnd, indirect, varTok->varId(), /*globalvar*/ true, settings)) + return false; + if (isTemporary(dot, &settings.library, /*unknown*/ true)) + return false; + } + if (exprDependsOnThis(tok->previous())) + return false; + const Function* func = tok->previous()->function(); + if (func && func->tokenDef->strAt(-1) == "&") { + const Scope* fScope = func->functionScope; + if (fScope && fScope->bodyEnd && Token::Match(fScope->bodyEnd->tokAt(-3), "return %var% ;")) { + const Token* varTok = fScope->bodyEnd->tokAt(-2); + if (isLargeObject(varTok->variable(), settings)) + return true; + } + } + return false; +} + +static bool checkVariableAssignment(const Token* tok, const Settings& settings) +{ + if (!Token::Match(tok, "%var% ;")) + return false; + const Variable* var = tok->variable(); + if (!var || !isLargeObject(var, settings)) + return false; + if (findVariableChanged(tok->tokAt(2), tok->scope()->bodyEnd, /*indirect*/ 0, var->declarationId(), /*globalvar*/ false, settings)) + return false; + if (var->isLocal() || (var->isArgument() && !var->isReference())) + return true; + const Scope* scope = tok->scope(); + while (scope && scope->type != ScopeType::eFunction) + scope = scope->nestedIn; + return scope && scope->function && (!scope->functionOf || scope->function->isConst() || scope->function->isStatic()); +} + void CheckOther::checkRedundantCopy() { if (!mSettings->severity.isEnabled(Severity::performance) || mTokenizer->isC() || !mSettings->certainty.isEnabled(Certainty::inconclusive)) @@ -3326,35 +3379,9 @@ void CheckOther::checkRedundantCopy() const Token* tok = startTok->next()->astOperand2(); if (!tok) continue; - if (!Token::Match(tok->previous(), "%name% (")) - continue; - if (!Token::Match(tok->link(), ") )|}| ;")) // bailout for usage like "const A a = getA()+3" + if (!checkFunctionReturnsRef(tok, *mSettings) && !checkVariableAssignment(tok, *mSettings)) continue; - - const Token* dot = tok->astOperand1(); - if (Token::simpleMatch(dot, ".")) { - const Token* varTok = dot->astOperand1(); - const int indirect = varTok->valueType() ? varTok->valueType()->pointer : 0; - if (isVariableChanged(tok, tok->scope()->bodyEnd, indirect, varTok->varId(), /*globalvar*/ true, *mSettings)) - continue; - if (isTemporary(dot, &mSettings->library, /*unknown*/ true)) - continue; - } - if (exprDependsOnThis(tok->previous())) - continue; - - const Function* func = tok->previous()->function(); - if (func && func->tokenDef->strAt(-1) == "&") { - const Scope* fScope = func->functionScope; - if (fScope && fScope->bodyEnd && Token::Match(fScope->bodyEnd->tokAt(-3), "return %var% ;")) { - const Token* varTok = fScope->bodyEnd->tokAt(-2); - if (varTok->variable() && !varTok->variable()->isGlobal() && - (!varTok->variable()->type() || !varTok->variable()->type()->classScope || - (varTok->variable()->valueType() && - varTok->variable()->valueType()->getSizeOf(*mSettings, ValueType::Accuracy::LowerBound, ValueType::SizeOf::Pointer) > 2 * mSettings->platform.sizeof_pointer))) - redundantCopyError(startTok, startTok->str()); - } - } + redundantCopyError(startTok, startTok->str()); } } diff --git a/test/testother.cpp b/test/testother.cpp index 21a640a3c7d..eec848ad163 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -2702,7 +2702,9 @@ class TestOther : public TestFixture { check("void f(std::string str) {\n" " std::string s2 = str;\n" "}"); - ASSERT_EQUALS("[test.cpp:1:20]: (performance) Function parameter 'str' should be passed by const reference. [passedByValue]\n", errout_str()); + ASSERT_EQUALS("[test.cpp:2:17]: (performance, inconclusive) Use const reference for 's2' to avoid unnecessary data copying. [redundantCopyLocalConst]\n" + "[test.cpp:1:20]: (performance) Function parameter 'str' should be passed by const reference. [passedByValue]\n", + errout_str()); check("void f(std::string str) {\n" " std::string& s2 = str;\n" @@ -9884,6 +9886,43 @@ class TestOther : public TestFixture { " if (s.empty()) {}\n" "}\n"); ASSERT_EQUALS("[test.cpp:6:16]: (performance, inconclusive) Use const reference for 's' to avoid unnecessary data copying. [redundantCopyLocalConst]\n", errout_str()); + + check("void f1(const std::string& s) {\n" + " std::string s1 = s;\n" + " (void)s1;\n" + "}\n" + "void f2() {\n" + " const std::string s;\n" + " std::string s1 = s;\n" + " (void)s1;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:2:17]: (performance, inconclusive) Use const reference for 's1' to avoid unnecessary data copying. [redundantCopyLocalConst]\n" + "[test.cpp:7:17]: (performance, inconclusive) Use const reference for 's1' to avoid unnecessary data copying. [redundantCopyLocalConst]\n", + errout_str()); + + check("struct S {\n" + " std::string m;\n" + " int f(const std::string& s);\n" + "};\n" + "int S::f(const std::string& s) {\n" + " std::string c = s;\n" + " m.clear();\n" + " return c.size();\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + check("struct S {\n" + " std::string m;\n" + " int f(std::string s);\n" + "};\n" + "int S::f(std::string s) {\n" + " s += m;\n" + " std::string c = s;\n" + " m.clear();\n" + " return c.size();\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:7:17]: (performance, inconclusive) Use const reference for 'c' to avoid unnecessary data copying. [redundantCopyLocalConst]\n", + errout_str()); } void checkNegativeShift() {