diff --git a/lib/checkother.cpp b/lib/checkother.cpp index 33f59654f..b07c4ace5 100644 --- a/lib/checkother.cpp +++ b/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."); +} diff --git a/lib/checkother.h b/lib/checkother.h index ecc015720..85001cf67 100644 --- a/lib/checkother.h +++ b/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" diff --git a/test/testother.cpp b/test/testother.cpp index c9818fcbf..0ae96fbc7 100644 --- a/test/testother.cpp +++ b/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)