add a performance checker for const assignment

lib/checkother.cpp

@@ -3004,3 +3004,52 @@ void CheckOther::unsignedPositiveError(const Token *tok, const std::string &varn
                     "An unsigned variable '" + varname + "' can't be negative so it is unnecessary to test it.");
     }
 }
+
+/*
+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.
+*/
+void CheckOther::checkRedundantCopy()
+{
+    if (!_settings->isEnabled("performance"))
+        return;
+
+    const SymbolDatabase *symbolDatabase = _tokenizer->getSymbolDatabase();
+
+    for (const Token *tok = _tokenizer->tokens(); tok; tok=tok->next()) {
+        const char *expect_end_token;
+        if (Token::Match(tok, "const %type% %var% =")) {
+            //match "const A a =" usage
+            expect_end_token = ";";
+        } else if (Token::Match(tok, "const %type% %var% (")) {
+            //match "const A a (" usage
+            expect_end_token = ")";
+        } else {
+            continue;
+        }
+
+        if (tok->strAt(1) == tok->strAt(4)) //avoid "const A a = A();"
+            continue;
+        if (!symbolDatabase->isClassOrStruct(tok->next()->str())) //avoid when %type% is standard type
+            continue;
+        const Token *var_tok = tok->tokAt(2);
+        tok = tok->tokAt(4);
+        while (tok &&Token::Match(tok,"%var% ."))
+            tok = tok->tokAt(2);
+        if (!Token::Match(tok, "%var% ("))
+            break;
+        const Token *match_end = (tok->next()->link()!=NULL)?tok->next()->link()->next():NULL;
+        if (match_end==NULL || !Token::Match(match_end,expect_end_token)) //avoid usage like "const A a = getA()+3"
+            break;
+        const Token *fToken = _tokenizer->getFunctionTokenByName(tok->str().c_str());
+        if (fToken &&fToken->previous() && fToken->previous()->str() == "&") {
+            redundantCopyError(var_tok,var_tok->str());
+        }
+    }
+}
+void CheckOther::redundantCopyError(const Token *tok,const std::string& varname)
+{
+    reportError(tok, Severity::performance,"redundantCopyLocalConst",
+                "Use const reference for "+varname+" to avoid unnecessary data copying.\n"
+                "The const "+varname+" gets a copy of the data since const reference is not used. You can avoid the unnecessary data copying by converting "+varname+" to const reference instead of just const.");
+}

lib/checkother.h

@@ -103,6 +103,7 @@ public:
         checkOther.checkAssignBoolToPointer();
         checkOther.checkBitwiseOnBoolean();
         checkOther.checkDoubleFree();
+        checkOther.checkRedundantCopy();
     }
 
     /** @brief Clarify calculation for ".. a * b ? .." */
@@ -235,6 +236,7 @@ public:
     /** @brief %Check for double free or double close operations */
     void checkDoubleFree();
     void doubleFreeError(const Token *tok, const std::string &varname);
+    void checkRedundantCopy();
 
 private:
     // Error messages..
@@ -290,6 +292,8 @@ private:
     void doubleCloseDirError(const Token *tok, const std::string &varname);
     void moduloAlwaysTrueFalseError(const Token* tok, const std::string& maxVal);
 
+    void redundantCopyError(const Token *tok, const std::string &varname);
+
     void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const {
         CheckOther c(0, settings, errorLogger);
 
@@ -306,6 +310,9 @@ private:
         c.doubleFreeError(0, "varname");
         c.invalidPointerCastError(0, "float", "double", false);
 
+        //performance
+        c.redundantCopyError(0, "varname");
+
         // style/warning
         c.cstyleCastError(0);
         c.dangerousUsageStrtolError(0);
@@ -364,6 +371,9 @@ private:
                "* incorrect length arguments for 'substr' and 'strncmp'\n"
                "* double free() or double closedir()\n"
 
+               //performance
+               "* redundant data copying for const variable\n"
+
                // style
                "* C-style pointer cast in cpp file\n"
                "* casting between incompatible pointer types\n"

test/testother.cpp

@@ -156,6 +156,8 @@ private:
         TEST_CASE(checkForSuspiciousSemicolon2);
 
         TEST_CASE(checkDoubleFree);
+
+        TEST_CASE(checkRedundantCopy);
     }
 
     void check(const char code[], const char *filename = NULL, bool experimental = false, bool inconclusive = true) {
@@ -5255,6 +5257,92 @@ private:
         );
         ASSERT_EQUALS("[test.cpp:8]: (error) Memory pointed to by 'p' is freed twice.\n", errout.str());
     }
+
+    void check_redundant_copy(const char code[]) {
+        // Clear the error buffer..
+        errout.str("");
+
+        Settings settings;
+        settings.addEnabled("performance");
+
+        // Tokenize..
+        Tokenizer tokenizer(&settings, this);
+        std::istringstream istr(code);
+        tokenizer.tokenize(istr, "test.cpp");
+
+        // Simplify token list..
+        CheckOther checkOther(&tokenizer, &settings, this);
+        tokenizer.simplifyTokenList();
+        checkOther.checkRedundantCopy();
+    }
+    void checkRedundantCopy() {
+        check_redundant_copy("class A{public:A(){}};\n"
+                             "const A& getA(){static A a;return a;}\n"
+                             "int main()\n"
+                             "{\n"
+                             "    const A a = getA();\n"
+                             "    return 0;\n"
+                             "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (performance) Use const reference for a to avoid unnecessary data copying.\n", errout.str());
+
+        check_redundant_copy("const int& getA(){static int a;return a;}\n"
+                             "int main()\n"
+                             "{\n"
+                             "    const int a = getA();\n"
+                             "    return 0;\n"
+                             "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_redundant_copy("const int& getA(){static int a;return a;}\n"
+                             "int main()\n"
+                             "{\n"
+                             "    int getA = 0;\n"
+                             "    const int a = getA + 3;\n"
+                             "    return 0;\n"
+                             "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_redundant_copy("class A{public:A(){}};\n"
+                             "const A& getA(){static A a;return a;}\n"
+                             "int main()\n"
+                             "{\n"
+                             "    const A a(getA());\n"
+                             "    return 0;\n"
+                             "}\n");
+        ASSERT_EQUALS("[test.cpp:5]: (performance) Use const reference for a to avoid unnecessary data copying.\n", errout.str());
+
+        check_redundant_copy("const int& getA(){static int a;return a;}\n"
+                             "int main()\n"
+                             "{\n"
+                             "    const int a(getA());\n"
+                             "    return 0;\n"
+                             "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_redundant_copy("class A{\n"
+                             "public:A(int a=0){_a = a;}\n"
+                             "A operator+(const A & a){return A(_a+a._a);}\n"
+                             "private:int _a;};\n"
+                             "const A& getA(){static A a;return a;}\n"
+                             "int main()\n"
+                             "{\n"
+                             "    const A a = getA() + 1;\n"
+                             "    return 0;\n"
+                             "}\n");
+        ASSERT_EQUALS("", errout.str());
+
+        check_redundant_copy("class A{\n"
+                             "public:A(int a=0){_a = a;}\n"
+                             "A operator+(const A & a){return A(_a+a._a);}\n"
+                             "private:int _a;};\n"
+                             "const A& getA(){static A a;return a;}\n"
+                             "int main()\n"
+                             "{\n"
+                             "    const A a(getA()+1);\n"
+                             "    return 0;\n"
+                             "}\n");
+        ASSERT_EQUALS("", errout.str());
+    }
 };
 
 REGISTER_TEST(TestOther)