From 629504450d0fab9497266f05795008f58a97401a Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Mon, 23 Feb 2026 19:10:35 +0100 Subject: [PATCH 1/6] Fix #12609 FN redundantCopyLocalConst (copy from variable) --- lib/checkother.cpp | 85 +++++++++++++++++++++++++++++++--------------- test/testother.cpp | 4 ++- 2 files changed, 60 insertions(+), 29 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index cf5ecb65b91..7dbee48d392 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3290,10 +3290,65 @@ 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 (isVariableChanged(var, settings)) + return false; + if (var->isMember()) { + const Scope* scope = tok->scope(); + while (scope && scope->type != ScopeType::eFunction) + scope = scope->nestedIn; + if (!scope || !scope->function || (!scope->function->isConst() && !scope->function->isStatic())) + return false; + } + return true; +} + void CheckOther::checkRedundantCopy() { if (!mSettings->severity.isEnabled(Severity::performance) || mTokenizer->isC() || !mSettings->certainty.isEnabled(Certainty::inconclusive)) @@ -3326,35 +3381,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..435976bd8be 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" From 871b7e6c122da81193322d107ea4b39ac994b1f4 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Mon, 23 Feb 2026 19:20:23 +0100 Subject: [PATCH 2/6] Add test --- test/testother.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/testother.cpp b/test/testother.cpp index 435976bd8be..198afa7ddfe 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -9886,6 +9886,19 @@ 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()); } void checkNegativeShift() { From 15e18829ad5e7a38b84c68323385701597df8b85 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Mon, 23 Feb 2026 19:22:39 +0100 Subject: [PATCH 3/6] Format --- lib/checkother.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 7dbee48d392..57860b8e20c 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3294,7 +3294,7 @@ 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)); + (var->valueType() && var->valueType()->getSizeOf(settings, ValueType::Accuracy::LowerBound, ValueType::SizeOf::Pointer) > 2 * settings.platform.sizeof_pointer)); } //--------------------------------------------------------------------------- From 73f8b9c8807f92d739476eca700f545b05941e48 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Mon, 23 Feb 2026 20:26:52 +0100 Subject: [PATCH 4/6] Fix --- lib/checkother.cpp | 12 +++++------- test/testother.cpp | 11 +++++++++++ 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 57860b8e20c..a894714e673 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3339,13 +3339,11 @@ static bool checkVariableAssignment(const Token* tok, const Settings& settings) return false; if (isVariableChanged(var, settings)) return false; - if (var->isMember()) { - const Scope* scope = tok->scope(); - while (scope && scope->type != ScopeType::eFunction) - scope = scope->nestedIn; - if (!scope || !scope->function || (!scope->function->isConst() && !scope->function->isStatic())) - return false; - } + const Scope* scope = tok->scope(); + while (scope && scope->type != ScopeType::eFunction) + scope = scope->nestedIn; + if (!scope || !scope->function || (scope->functionOf && !scope->function->isConst() && !scope->function->isStatic())) + return false; return true; } diff --git a/test/testother.cpp b/test/testother.cpp index 198afa7ddfe..e6b3edefb68 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -9899,6 +9899,17 @@ class TestOther : public TestFixture { 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()); } void checkNegativeShift() { From 392f09efbbcd0bf7f86495ad237565043c9759a9 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Mon, 23 Feb 2026 20:45:48 +0100 Subject: [PATCH 5/6] Simplify --- lib/checkother.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index a894714e673..a3010567a8a 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3342,9 +3342,7 @@ static bool checkVariableAssignment(const Token* tok, const Settings& settings) const Scope* scope = tok->scope(); while (scope && scope->type != ScopeType::eFunction) scope = scope->nestedIn; - if (!scope || !scope->function || (scope->functionOf && !scope->function->isConst() && !scope->function->isStatic())) - return false; - return true; + return scope && scope->function && (!scope->functionOf || scope->function->isConst() || scope->function->isStatic()); } void CheckOther::checkRedundantCopy() From 3de31556a3c2dbe5d28abb916bff43d0ce5f9869 Mon Sep 17 00:00:00 2001 From: chrchr-github Date: Mon, 23 Feb 2026 20:58:15 +0100 Subject: [PATCH 6/6] Fix --- lib/checkother.cpp | 4 +++- test/testother.cpp | 13 +++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index a3010567a8a..ee7e643cf01 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -3337,8 +3337,10 @@ static bool checkVariableAssignment(const Token* tok, const Settings& settings) const Variable* var = tok->variable(); if (!var || !isLargeObject(var, settings)) return false; - if (isVariableChanged(var, settings)) + 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; diff --git a/test/testother.cpp b/test/testother.cpp index e6b3edefb68..eec848ad163 100644 --- a/test/testother.cpp +++ b/test/testother.cpp @@ -9910,6 +9910,19 @@ class TestOther : public TestFixture { " 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() {