Fix #12609 FN redundantCopyLocalConst (copy from variable)

Author: web-flow

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());
     }
 }
 

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"