From fdcb6634dfac332cbebc50f2448c7122a30386fa Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Marjam=C3=A4ki?= <daniel.marjamaki@gmail.com>
Date: Sat, 14 Dec 2013 08:46:18 +0100
Subject: [PATCH] Fixed #5190 (FP Use const reference to avoid data copying)

---
 lib/checkother.cpp | 28 ++++++++++++++++++++++------
 test/testother.cpp |  9 +++++++++
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/lib/checkother.cpp b/lib/checkother.cpp
index 26993eb00..33f93e591 100644
--- a/lib/checkother.cpp
+++ b/lib/checkother.cpp
@@ -3256,6 +3256,21 @@ void CheckOther::pointerPositiveError(const Token *tok, bool inconclusive)
                 "A pointer can not be negative so it is either pointless or an error to check if it is not.", inconclusive);
 }
 
+/* check if a constructor in given class scope takes a reference */
+static bool constructorTakesReference(const Scope * const classScope)
+{
+    for (std::list<Function>::const_iterator func = classScope->functionList.begin(); func != classScope->functionList.end(); ++func) {
+        if (func->isConstructor()) {
+            const Function &constructor = *func;
+            for (std::size_t argnr = 0U; argnr < constructor.argCount(); argnr++) {
+                if (constructor.getArgumentVar(argnr)->isReference())
+                    return true;
+            }
+        }
+    }
+    return false;
+}
+
 /*
 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.
@@ -3274,12 +3289,13 @@ void CheckOther::checkRedundantCopy()
             continue;
 
         const Token* startTok = var->nameToken();
-        const Token* endToken;
         if (startTok->strAt(1) == "=") // %type% %var% = ... ;
-            endToken = Token::findsimplematch(startTok->tokAt(2), ";");
-        else if (startTok->strAt(1) == "(") // %type% %var%(...)
-            endToken = startTok->linkAt(1);
-        else
+            ;
+        else if (startTok->strAt(1) == "(" && var->isClass() && var->typeScope()) {
+            // Object is instantiated. Warn if constructor takes arguments by value.
+            if (constructorTakesReference(var->typeScope()))
+                continue;
+        } else
             continue;
 
         const Token* tok = startTok->tokAt(2);
@@ -3287,7 +3303,7 @@ void CheckOther::checkRedundantCopy()
             tok = tok->tokAt(2);
         if (!Token::Match(tok, "%var% ("))
             continue;
-        if (tok->linkAt(1)->next() != endToken) // bailout for usage like "const A a = getA()+3"
+        if (!Token::Match(tok->linkAt(1), ") )| ;")) // bailout for usage like "const A a = getA()+3"
             continue;
 
         const Function* func = tok->function();
diff --git a/test/testother.cpp b/test/testother.cpp
index e74d38385..73a2456ff 100644
--- a/test/testother.cpp
+++ b/test/testother.cpp
@@ -6122,6 +6122,15 @@ private:
                              "    return 0;\n"
                              "}");
         ASSERT_EQUALS("", errout.str());
+
+        // #5190 - FP when creating object with constructor that takes a reference
+        check_redundant_copy("class A {};\n"
+                             "class B { B(const A &a); };\n"
+                             "const A &getA();\n"
+                             "void f() {\n"
+                             "    const B b(getA());\n"
+                             "}");
+        ASSERT_EQUALS("", errout.str());
     }
 
     void checkNegativeShift() {