Fix #12609 FN redundantCopyLocalConst (copy from variable) (#8251)

Co-authored-by: chrchr-github <noreply@github.com>

Author: chrchr-github

lib/checkother.cpp

@@ -3292,10 +3292,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))
@@ -3328,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());
     }
 }
 

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"
@@ -9905,6 +9907,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() {