Improved check: Detect passedByValue even for arguments that are not declared as "const"

lib/checkother.cpp

@@ -1383,9 +1383,39 @@ void CheckOther::commaSeparatedReturnError(const Token *tok)
 }
 
 //---------------------------------------------------------------------------
-// Check for constant function parameters
+// Check for function parameters that should be passed by reference
 //---------------------------------------------------------------------------
-void CheckOther::checkConstantFunctionParameter()
+static std::size_t estimateSize(const Type* type, const Settings* settings, const SymbolDatabase* symbolDatabase, std::size_t recursionDepth = 0)
+{
+    if (recursionDepth > 20)
+        return 0;
+
+    std::size_t cumulatedSize = 0;
+    for (std::list<Variable>::const_iterator i = type->classScope->varlist.cbegin(); i != type->classScope->varlist.cend(); ++i) {
+        std::size_t size = 0;
+        if (i->isStatic())
+            continue;
+        if (i->isPointer() || i->isReference())
+            size = settings->sizeof_pointer;
+        else if (i->type() && i->type()->classScope)
+            size = estimateSize(i->type(), settings, symbolDatabase, recursionDepth+1);
+        else if (i->isStlStringType() || (i->isStlType() && Token::Match(i->typeStartToken(), "std :: %type% <") && !Token::simpleMatch(i->typeStartToken()->linkAt(3), "> ::")))
+            size = 16; // Just guess
+        else
+            size = symbolDatabase->sizeOfType(i->typeStartToken());
+
+        if (i->isArray())
+            cumulatedSize += size*i->dimension(0);
+        else
+            cumulatedSize += size;
+    }
+    for (std::vector<Type::BaseInfo>::const_iterator i = type->derivedFrom.cbegin(); i != type->derivedFrom.cend(); ++i)
+        if (i->type && i->type->classScope)
+            cumulatedSize += estimateSize(i->type, settings, symbolDatabase, recursionDepth+1);
+    return cumulatedSize;
+}
+
+void CheckOther::checkPassByReference()
 {
     if (!_settings->isEnabled("performance") || _tokenizer->isC())
         return;
@@ -1394,34 +1424,97 @@ void CheckOther::checkConstantFunctionParameter()
 
     for (unsigned int i = 1; i < symbolDatabase->getVariableListSize(); i++) {
         const Variable* var = symbolDatabase->getVariableFromVarId(i);
-        if (!var || !var->isArgument() || !var->isClass() || !var->isConst() || var->isPointer() || var->isArray() || var->isReference() || var->isEnumType())
+        if (!var || !var->isArgument() || !var->isClass() || var->isPointer() || var->isArray() || var->isReference() || var->isEnumType())
             continue;
 
         if (var->scope() && var->scope()->function->arg->link()->strAt(-1) == ".")
             continue; // references could not be used as va_start parameters (#5824)
 
+        bool inconclusive = false;
+
         const Token* const tok = var->typeStartToken();
-        // TODO: False negatives. This pattern only checks for string.
-        //       Investigate if there are other classes in the std
-        //       namespace and add them to the pattern. There are
-        //       streams for example (however it seems strange with
-        //       const stream parameter).
         if (var->isStlStringType()) {
-            passedByValueError(tok, var->name());
+            ;
         } else if (var->isStlType() && Token::Match(tok, "std :: %type% <") && !Token::simpleMatch(tok->linkAt(3), "> ::")) {
-            passedByValueError(tok, var->name());
+            ;
-        } else if (var->type()) {  // Check if type is a struct or class.
+        } else if (var->type() && !var->type()->isEnumType()) { // Check if type is a struct or class.
-            passedByValueError(tok, var->name());
+            // Ensure that it is a large object.
+            if (!var->type()->classScope)
+                inconclusive = true;
+            else if (estimateSize(var->type(), _settings, symbolDatabase) <= 8)
+                continue;
+        } else
+            continue;
+
+        if (inconclusive && !_settings->inconclusive)
+            continue;
+
+        bool isConst = var->isConst();
+        if (!isConst) {
+            // Check if variable could be const
+            if (!var->scope() || var->scope()->function->isVirtual())
+                continue;
+
+            isConst = true;
+            for (const Token* tok2 = var->scope()->classStart; tok2 != var->scope()->classEnd; tok2 = tok2->next()) {
+                if (tok2->varId() == var->declarationId()) {
+                    const Token* parent = tok2->astParent();
+                    if (!parent)
+                        ;
+                    else if (parent->str() == "<<" || parent->str() == ">>") {
+                        if (parent->str() == "<<" && parent->astOperand1() == tok2)
+                            isConst = false;
+                        else if (parent->str() == ">>" && parent->astOperand2() == tok2)
+                            isConst = false;
+                    } else if (parent->str() == "," || parent->str() == "(") { // function argument
+                        const Token* tok3 = tok2->previous();
+                        unsigned int argNr = 0;
+                        while (tok3 && tok3->str() != "(") {
+                            if (tok3->link() && Token::Match(tok3, ")|]|}|>"))
+                                tok3 = tok3->link();
+                            else if (tok->link())
+                                break;
+                            else if (tok3->str() == ";")
+                                break;
+                            else if (tok3->str() == ",")
+                                argNr++;
+                            tok3 = tok3->previous();
+                        }
+                        if (!tok3 || tok3->str() != "(" || !tok3->astOperand1() || !tok3->astOperand1()->function())
+                            isConst = false;
+                        else {
+                            const Variable* argVar = tok3->astOperand1()->function()->getArgumentVar(argNr);
+                            if (!argVar|| (!argVar->isConst() && argVar->isReference()))
+                                isConst = false;
+                        }
+                    } else if (parent->isConstOp())
+                        ;
+                    else if (Token::Match(tok2, "%var% . %name% (")) {
+                        const Function* func = tok2->tokAt(2)->function();
+                        if (func && (func->isConst() || func->isStatic()))
+                            ;
+                        else
+                            isConst = false;
+                    } else
+                        isConst = false;
+
+                    if (!isConst)
+                        break;
                 }
             }
         }
 
-void CheckOther::passedByValueError(const Token *tok, const std::string &parname)
+        if (isConst)
+            passedByValueError(tok, var->name(), inconclusive);
+    }
+}
+
+void CheckOther::passedByValueError(const Token *tok, const std::string &parname, bool inconclusive)
 {
     reportError(tok, Severity::performance, "passedByValue",
                 "Function parameter '" + parname + "' should be passed by reference.\n"
                 "Parameter '" +  parname + "' is passed by value. It could be passed "
-                "as a (const) reference which is usually faster and recommended in C++.", CWE398, false);
+                "as a (const) reference which is usually faster and recommended in C++.", CWE398, inconclusive);
 }
 
 //---------------------------------------------------------------------------

lib/checkother.h

@@ -81,7 +81,7 @@ public:
         // Checks
         checkOther.clarifyCalculation();
         checkOther.clarifyStatement();
-        checkOther.checkConstantFunctionParameter();
+        checkOther.checkPassByReference();
         checkOther.checkIncompleteStatement();
         checkOther.checkCastIntToCharAndBack();
 
@@ -115,8 +115,8 @@ public:
     /** @brief %Check for comma separated statements in return */
     void checkCommaSeparatedReturn();
 
-    /** @brief %Check for constant function parameter */
+    /** @brief %Check for function parameters that should be passed by reference */
-    void checkConstantFunctionParameter();
+    void checkPassByReference();
 
     /** @brief Using char variable as array index / as operand in bit operation */
     void checkCharVariable();
@@ -212,7 +212,7 @@ private:
     void clarifyStatementError(const Token* tok);
     void cstyleCastError(const Token *tok);
     void invalidPointerCastError(const Token* tok, const std::string& from, const std::string& to, bool inconclusive);
-    void passedByValueError(const Token *tok, const std::string &parname);
+    void passedByValueError(const Token *tok, const std::string &parname, bool inconclusive);
     void constStatementError(const Token *tok, const std::string &type);
     void signedCharArrayIndexError(const Token *tok);
     void unknownSignCharArrayIndexError(const Token *tok);
@@ -274,7 +274,7 @@ private:
         c.checkComparisonFunctionIsAlwaysTrueOrFalseError(nullptr, "isless","varName",false);
         c.checkCastIntToCharAndBackError(nullptr, "func_name");
         c.cstyleCastError(nullptr);
-        c.passedByValueError(nullptr,  "parametername");
+        c.passedByValueError(nullptr,  "parametername", false);
         c.constStatementError(nullptr,  "type");
         c.signedCharArrayIndexError(nullptr);
         c.unknownSignCharArrayIndexError(nullptr);
@@ -335,6 +335,7 @@ private:
                // performance
                "- redundant data copying for const variable\n"
                "- subsequent assignment or copying to a variable or buffer\n"
+               "- passing parameter by value\n"
 
                // portability
                "- memset() with a float as the 2nd parameter\n"
@@ -343,7 +344,6 @@ private:
                // style
                "- C-style pointer cast in C++ code\n"
                "- casting between incompatible pointer types\n"
-               "- passing parameter by value\n"
                "- [Incomplete statement](IncompleteStatement)\n"
                "- [check how signed char variables are used](CharVar)\n"
                "- variable scope can be limited\n"

test/testother.cpp

@@ -82,6 +82,7 @@ private:
         TEST_CASE(invalidPointerCast);
 
         TEST_CASE(passedByValue);
+        TEST_CASE(passedByValue_nonConst);
 
         TEST_CASE(switchRedundantAssignmentTest);
         TEST_CASE(switchRedundantOperationTest);
@@ -1292,7 +1293,28 @@ private:
         check("void f(const std::string::size_type x) {}");
         ASSERT_EQUALS("", errout.str());
 
-        check("class Foo;\nvoid f(const Foo foo) {}");
+        check("class Foo;\nvoid f(const Foo foo) {}"); // Unknown class
+        ASSERT_EQUALS("[test.cpp:2]: (performance, inconclusive) Function parameter 'foo' should be passed by reference.\n", errout.str());
+
+        check("class Foo { std::vector<int> v; };\nvoid f(const Foo foo) {}"); // Large class (STL member)
+        ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by reference.\n", errout.str());
+
+        check("class Foo { int i; };\nvoid f(const Foo foo) {}"); // Small class
+        ASSERT_EQUALS("", errout.str());
+
+        check("class Foo { int i[6]; };\nvoid f(const Foo foo) {}"); // Large class (array)
+        ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by reference.\n", errout.str());
+
+        check("class Foo { std::string* s; };\nvoid f(const Foo foo) {}"); // Small class (pointer)
+        ASSERT_EQUALS("", errout.str());
+
+        check("class Foo { static std::string s; };\nvoid f(const Foo foo) {}"); // Small class (static member)
+        ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by reference.\n", errout.str());
+
+        check("class X { std::string s; }; class Foo : X { };\nvoid f(const Foo foo) {}"); // Large class (inherited)
+        ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by reference.\n", errout.str());
+
+        check("class X { std::string s; }; class Foo { X x; };\nvoid f(const Foo foo) {}"); // Large class (inherited)
         ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'foo' should be passed by reference.\n", errout.str());
 
         check("void f(const std::string &str) {}");
@@ -1341,10 +1363,105 @@ private:
               "}; ");
         ASSERT_EQUALS("", errout.str());
 
+        check("class X {\n"
+              "    virtual void func(const std::string str) {}\n"
+              "};");
+        ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str());
+    }
+
+    void passedByValue_nonConst() {
+        check("void f(std::string str) {}");
+        ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str());
+
+        check("void f(std::string str) {\n"
+              "    return str + x;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str());
+
+        check("void f(std::string str) {\n"
+              "    std::cout << str;\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:1]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str());
+
+        check("void f(std::string str) {\n"
+              "    std::cin >> str;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void f(std::string str) {\n"
+              "    foo(str);\n" // It could be that foo takes str as non-const-reference
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void foo(const std::string& str);\n"
+              "void f(std::string str) {\n"
+              "    foo(str);\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str());
+
+        check("void foo(std::string str);\n"
+              "void f(std::string str) {\n"
+              "    foo(str);\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str());
+
+        check("void foo(std::string& str);\n"
+              "void f(std::string str) {\n"
+              "    foo(str);\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("void foo(int& i1, const std::string& str, int& i2);\n"
+              "void f(std::string str) {\n"
+              "    foo((a+b)*c, str, x);\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str());
+
+        check("std::string f(std::string str) {\n"
+              "    str += x;\n"
+              "    return str;\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("class X {\n"
+              "    std::string s;\n"
+              "    void func() const;\n"
+              "};\n"
+              "Y f(X x) {\n"
+              "    x.func();\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:5]: (performance) Function parameter 'x' should be passed by reference.\n", errout.str());
+
+        check("class X {\n"
+              "    void func();\n"
+              "};\n"
+              "Y f(X x) {\n"
+              "    x.func();\n"
+              "}");
+        ASSERT_EQUALS("", errout.str());
+
+        check("class X {\n"
+              "    void func(std::string str) {}\n"
+              "};");
+        ASSERT_EQUALS("[test.cpp:2]: (performance) Function parameter 'str' should be passed by reference.\n", errout.str());
+
+        check("class X {\n"
+              "    virtual void func(std::string str) {}\n" // Do not warn about virtual functions, if 'str' is not declared as const
+              "};");
+        ASSERT_EQUALS("", errout.str());
+
+        check("class X {\n"
+              "    uint64_t i;\n"
+              "};\n"
+              "class Y : X {\n"
+              "    uint64_t j;\n"
+              "};\n"
+              "void f(X x, Y y) {\n"
+              "}");
+        ASSERT_EQUALS("[test.cpp:7]: (performance) Function parameter 'y' should be passed by reference.\n", errout.str());
     }
 
     void switchRedundantAssignmentTest() {
-
         check("void foo()\n"
               "{\n"
               "    int y = 1;\n"
@@ -3436,7 +3553,7 @@ private:
               "}");
         ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:2]: (style) Same expression on both sides of '|'.\n", errout.str());
 
-        check("void foo(std::string a, std::string b) {\n"
+        check("void foo(const std::string& a, const std::string& b) {\n"
               "  return a.find(b+\"&\") || a.find(\"&\"+b);\n"
               "}");
         ASSERT_EQUALS("", errout.str());