Fix #10704 FN redundantCopyLocalConst (#4115)

lib/astutils.cpp

@@ -2615,7 +2615,8 @@ bool isExpressionChanged(const Token* expr, const Token* start, const Token* end
         if (tok->variable()) {
             if (tok->variable()->isConst())
                 return false;
-            global = !tok->variable()->isLocal() && !tok->variable()->isArgument();
+            global = !tok->variable()->isLocal() && !tok->variable()->isArgument() &&
+                     !(tok->variable()->isMember() && !tok->variable()->isStatic());
         } else if (tok->isIncompleteVar() && !tok->isIncompleteConstant()) {
             global = true;
         }

lib/checkother.cpp

@@ -1181,10 +1181,12 @@ static int estimateSize(const Type* type, const Settings* settings, const Symbol
 
 static bool canBeConst(const Variable *var, const Settings* settings)
 {
+    if (!var->scope())
+        return false;
     {
         // check initializer list. If variable is moved from it can't be const.
         const Function* func_scope = var->scope()->function;
-        if (func_scope->type == Function::Type::eConstructor) {
+        if (func_scope && func_scope->type == Function::Type::eConstructor) {
             //could be initialized in initializer list
             if (func_scope->arg->link()->next()->str() == ":") {
                 for (const Token* tok2 = func_scope->arg->link()->next()->next(); tok2 != var->scope()->bodyStart; tok2 = tok2->next()) {
@@ -1204,6 +1206,11 @@ static bool canBeConst(const Variable *var, const Settings* settings)
         const Token* parent = tok2->astParent();
         if (!parent)
             continue;
+        if (Token::simpleMatch(tok2->next(), ";") && tok2->next()->isSplittedVarDeclEq()) {
+            tok2 = tok2->tokAt(2);
+            tok2 = Token::findsimplematch(tok2, ";");
+            continue;
+        }
         if (parent->str() == "<<" || isLikelyStreamRead(true, parent)) {
             if (parent->str() == "<<" && parent->astOperand1() == tok2)
                 return false;
@@ -2701,7 +2708,7 @@ void CheckOther::checkRedundantCopy()
     const SymbolDatabase *symbolDatabase = mTokenizer->getSymbolDatabase();
 
     for (const Variable* var : symbolDatabase->variableList()) {
-        if (!var || var->isReference() || !var->isConst() || var->isPointer() || (!var->type() && !var->isStlType())) // bailout if var is of standard type, if it is a pointer or non-const
+        if (!var || var->isReference() || (!var->isConst() && !canBeConst(var, mSettings)) || var->isPointer() || (!var->type() && !var->isStlType())) // bailout if var is of standard type, if it is a pointer or non-const
             continue;
 
         const Token* startTok = var->nameToken();
@@ -2711,6 +2718,8 @@ void CheckOther::checkRedundantCopy()
             // Object is instantiated. Warn if constructor takes arguments by value.
             if (constructorTakesReference(var->typeScope()))
                 continue;
+        } else if (Token::simpleMatch(startTok->next(), ";") && startTok->next()->isSplittedVarDeclEq()) {
+            startTok = startTok->tokAt(2);
         } else
             continue;
 
@@ -2723,17 +2732,27 @@ void CheckOther::checkRedundantCopy()
             continue;
 
         const Token* dot = tok->astOperand1();
-        if (Token::simpleMatch(dot, ".") && dot->astOperand1() && isVariableChanged(dot->astOperand1()->variable(), mSettings, mTokenizer->isCPP()))
+        if (Token::simpleMatch(dot, ".")) {
-            continue;
+            if (dot->astOperand1() && isVariableChanged(dot->astOperand1()->variable(), mSettings, mTokenizer->isCPP()))
+                continue;
+            if (isTemporary(/*cpp*/ true, dot, &mSettings->library, /*unknown*/ true))
+                continue;
+        }
         if (exprDependsOnThis(tok->previous()))
             continue;
 
         const Function* func = tok->previous()->function();
         if (func && func->tokenDef->strAt(-1) == "&") {
-            redundantCopyError(startTok, startTok->str());
+            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())
+                    redundantCopyError(startTok, startTok->str());
+            }
         }
     }
 }
+
 void CheckOther::redundantCopyError(const Token *tok,const std::string& varname)
 {
     reportError(tok, Severity::performance, "redundantCopyLocalConst",

lib/symboldatabase.cpp

@@ -2091,6 +2091,10 @@ Variable& Variable::operator=(const Variable &var)
     return *this;
 }
 
+bool Variable::isMember() const {
+    return mScope && mScope->isClassOrStructOrUnion();
+}
+
 bool Variable::isPointerArray() const
 {
     return isArray() && nameToken() && nameToken()->previous() && (nameToken()->previous()->str() == "*");

lib/symboldatabase.h

@@ -379,6 +379,12 @@ public:
         return (mAccess == AccessControl::Local) && !isExtern();
     }
 
+    /**
+     * Is variable a member of a user-defined type.
+     * @return true if member, false if not or unknown
+     */
+    bool isMember() const;
+
     /**
      * Is variable mutable.
      * @return true if mutable, false if not

lib/templatesimplifier.cpp

@@ -3899,7 +3899,6 @@ void TemplateSimplifier::simplifyTemplates(
                 continue;
             }
 
-            // cppcheck-suppress redundantCopyLocalConst ; False positive
             const std::string strop = op->str();
             const std::string strargs = (args && args->isName()) ? args->str() : "";
 

lib/tokenize.cpp

@@ -2334,7 +2334,7 @@ bool Tokenizer::simplifyUsing()
                 Token::Match(tok->linkAt(2), "] ] = ::| %name%")))))
             continue;
 
-        std::string name = tok->strAt(1);
+        const std::string& name = tok->strAt(1);
         const Token *nameToken = tok->next();
         std::string scope = currentScope->fullName;
         Token *usingStart = tok;
@@ -9518,7 +9518,7 @@ void Tokenizer::simplifyOperatorName()
 
     for (Token *tok = list.front(); tok; tok = tok->next()) {
         if (Token::Match(tok, "%op% %str% %name%")) {
-            std::string name = tok->strAt(2);
+            const std::string name = tok->strAt(2);
             Token * const str = tok->next();
             str->deleteNext();
             tok->insertToken("operator\"\"" + name);

lib/valueflow.cpp

@@ -5596,7 +5596,7 @@ static void valueFlowAfterSwap(TokenList* tokenlist,
                 continue;
             for (int i = 0; i < 2; i++) {
                 std::vector<const Variable*> vars = getVariables(args[0]);
-                std::list<ValueFlow::Value> values = args[0]->values();
+                const std::list<ValueFlow::Value>& values = args[0]->values();
                 valueFlowForwardAssign(args[0], args[1], vars, values, false, tokenlist, errorLogger, settings);
                 std::swap(args[0], args[1]);
             }
@@ -6190,7 +6190,7 @@ struct ConditionHandler {
                         parent = parent->astParent();
                     bool possible = false;
                     if (Token::Match(parent, "&&|%oror%")) {
-                        std::string op = parent->str();
+                        const std::string& op(parent->str());
                         while (parent && parent->str() == op)
                             parent = parent->astParent();
                         if (Token::simpleMatch(parent, "!") || Token::simpleMatch(parent, "== false"))
@@ -7732,7 +7732,7 @@ static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogge
                 if (Token::Match(tok, "%var% (|{") && tok->next()->astOperand2() &&
                     tok->next()->astOperand2()->str() != ",") {
                     Token* inTok = tok->next()->astOperand2();
-                    std::list<ValueFlow::Value> values = inTok->values();
+                    const std::list<ValueFlow::Value>& values = inTok->values();
                     const bool constValue = inTok->isNumber();
                     valueFlowForwardAssign(inTok, var, values, constValue, true, tokenlist, errorLogger, settings);
 
@@ -7760,7 +7760,7 @@ static void valueFlowSmartPointer(TokenList *tokenlist, ErrorLogger * errorLogge
                     Token* inTok = ftok->astOperand2();
                     if (!inTok)
                         continue;
-                    std::list<ValueFlow::Value> values = inTok->values();
+                    const std::list<ValueFlow::Value>& values = inTok->values();
                     valueFlowForwardAssign(inTok, tok, vars, values, false, tokenlist, errorLogger, settings);
                 }
             } else if (Token::simpleMatch(tok->astParent(), ". release ( )")) {

test/testother.cpp

@@ -6657,7 +6657,8 @@ private:
               "    if (i == j) {}\n"
               "}");
         ASSERT_EQUALS(
-            "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n",
+            "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n"
+            "[test.cpp:3] -> [test.cpp:4] -> [test.cpp:6]: (style) The comparison 'i == j' is always true because 'i' and 'j' represent the same value.\n",
             errout.str());
 
         check("struct A { int x; int y; };"
@@ -6669,7 +6670,8 @@ private:
               "    if (i == a.x) {}\n"
               "}");
         ASSERT_EQUALS(
-            "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n",
+            "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n"
+            "[test.cpp:3] -> [test.cpp:6]: (style) The comparison 'i == a.x' is always true because 'i' and 'a.x' represent the same value.\n",
             errout.str());
 
         check("struct A { int x; int y; };"
@@ -6681,7 +6683,8 @@ private:
               "    if (j == a.x) {}\n"
               "}");
         ASSERT_EQUALS(
-            "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n",
+            "[test.cpp:4] -> [test.cpp:3]: (style, inconclusive) Same expression used in consecutive assignments of 'i' and 'j'.\n"
+            "[test.cpp:4] -> [test.cpp:6]: (style) The comparison 'j == a.x' is always true because 'j' and 'a.x' represent the same value.\n",
             errout.str());
 
         // Issue #8612
@@ -7603,6 +7606,33 @@ private:
               "    }\n"
               "};\n", nullptr, /*experimental*/ false, /*inconclusive*/ true);
         ASSERT_EQUALS("", errout.str());
+
+        // #10704
+        check("struct C {\n"
+              "    std::string str;\n"
+              "    const std::string& get() const { return str; }\n"
+              "};\n"
+              "struct D {\n"
+              "    C c;\n"
+              "    bool f() const {\n"
+              "        std::string s = c.get();\n"
+              "        return s.empty();\n"
+              "    }\n"
+              "};\n");
+        ASSERT_EQUALS("[test.cpp:8]: (performance, inconclusive) Use const reference for 's' to avoid unnecessary data copying.\n", errout.str());
+
+        check("struct C {\n"
+              "    const std::string & get() const { return m; }\n"
+              "    std::string m;\n"
+              "};\n"
+              "C getC();\n"
+              "void f() {\n"
+              "    const std::string s = getC().get();\n"
+              "}\n"
+              "void g() {\n"
+              "    std::string s = getC().get();\n"
+              "}\n");
+        ASSERT_EQUALS("", errout.str());
     }
 
     void checkNegativeShift() {