From bef2caf4890b3b0ec3b46f797bc0953de07b49da Mon Sep 17 00:00:00 2001 From: PKEuS Date: Wed, 4 Apr 2012 19:40:28 +0200 Subject: [PATCH] Improved checks in CheckStl: - Improved message of stlIfStrFind according to discussion on github (77d9ed18775bd2181ec518b1e7d2c842bb3da644) - Generalized pattern for substr in CheckStl::uselessCalls; added check for substr calls like ".substr(%any%,0)" which result in an empty string. --- lib/checkstl.cpp | 45 ++++++++++++++++++++------------------------- lib/checkstl.h | 4 ++-- test/teststl.cpp | 18 ++++++++++-------- 3 files changed, 32 insertions(+), 35 deletions(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 392bc587c..ed9c03c8a 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -832,12 +832,12 @@ void CheckStl::if_find() void CheckStl::if_findError(const Token *tok, bool str) { if (str) - reportError(tok, Severity::warning, "stlIfStrFind", - "Suspicious checking of string::find() return value.\n" - "Checking of string::find() return value looks Suspicious. " - "string::find will return 0 if the string is found at position 0. " - "If that is wanted to check then string::compare is a faster alternative " - "because it doesn't scan through the string."); + reportError(tok, Severity::performance, "stlIfStrFind", + "Inefficient usage of string::find in condition; string::compare would be faster.\n" + "Either inefficent or wrong usage of string::find. string::compare will be faster if " + "string::find's result is compared with 0, because it will not scan the whole " + "string. If your intention is to check that there are no findings in the string, " + "you should compare with string::npos."); else reportError(tok, Severity::warning, "stlIfFind", "Suspicious condition. The result of find is an iterator, but it is not properly checked."); } @@ -1317,24 +1317,19 @@ void CheckStl::autoPointerArrayError(const Token *tok) void CheckStl::uselessCalls() { for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { - if (tok->varId() == 0) - continue; - /*if (Token::Match(tok, "%var% . compare (") && - tok->varId() == tok->linkAt(3)->previous()->varId()) { - uselessCallsReturnValueError(tok, tok->tokAt(2)); - } else */ - if (Token::Match(tok, "%var% . compare|find|rfind|find_first_not_of|find_first_of|find_last_not_of|find_last_of ( %var% [,)]") && + if (tok->varId() && Token::Match(tok, "%var% . compare|find|rfind|find_first_not_of|find_first_of|find_last_not_of|find_last_of ( %var% [,)]") && tok->varId() == tok->tokAt(4)->varId()) { uselessCallsReturnValueError(tok->tokAt(4), tok->str(), tok->strAt(2)); - } else if (Token::Match(tok, "%var% . swap ( %var% )") && + } else if (tok->varId() && Token::Match(tok, "%var% . swap ( %var% )") && tok->varId() == tok->tokAt(4)->varId()) { uselessCallsSwapError(tok, tok->str()); - } else if (Token::Match(tok, "%var% . substr ( )")) { - uselessCallsSubstrError(tok, tok->str()); - } else if (Token::Match(tok, "%var% . substr ( 0")) { - if (tok->strAt(5) == ")" || - tok->linkAt(3)->strAt(-1) == "npos") - uselessCallsSubstrError(tok, tok->str()); + } else if (Token::Match(tok, ". substr (")) { + if (Token::Match(tok->tokAt(3), "0| )")) + uselessCallsSubstrError(tok, false); + else if (tok->strAt(3) == "0" && tok->linkAt(2)->strAt(-1) == "npos") + uselessCallsSubstrError(tok, false); + else if (Token::Match(tok->linkAt(2)->tokAt(-2), ", 0 )")) + uselessCallsSubstrError(tok, true); } } } @@ -1361,10 +1356,10 @@ void CheckStl::uselessCallsSwapError(const Token *tok, const std::string &varnam reportError(tok, Severity::performance, "uselessCallsSwap", errmsg.str()); } -void CheckStl::uselessCallsSubstrError(const Token *tok, const std::string &varname) +void CheckStl::uselessCallsSubstrError(const Token *tok, bool empty) { - std::ostringstream errmsg; - errmsg << "Function \'substr\' useless call. Function create copy of the \'" << varname << "\' object.\n" << - "Function \'substr\' useless call. \'substr\' function create copy of the whole \'" << varname << "\' object which can decrease in performance. Please use \'=\' operator instead."; - reportError(tok, Severity::performance, "uselessCallsSubstr", errmsg.str()); + if (empty) + reportError(tok, Severity::performance, "uselessCallsSubstr", "Useless call of function 'substr' because it returns an empty string."); + else + reportError(tok, Severity::performance, "uselessCallsSubstr", "Useless call of function 'substr' because it returns a copy of the object. Use operator= instead."); } diff --git a/lib/checkstl.h b/lib/checkstl.h index 17a29080c..8b20af7c9 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -173,7 +173,7 @@ private: void uselessCallsReturnValueError(const Token *tok, const std::string &varname, const std::string &function); void uselessCallsSwapError(const Token *tok, const std::string &varname); - void uselessCallsSubstrError(const Token *tok, const std::string &varname); + void uselessCallsSubstrError(const Token *tok, bool empty); void getErrorMessages(ErrorLogger *errorLogger, const Settings *settings) const { CheckStl c(0, settings, errorLogger); @@ -198,7 +198,7 @@ private: c.autoPointerArrayError(0); c.uselessCallsReturnValueError(0, "str", "find"); c.uselessCallsSwapError(0, "str"); - c.uselessCallsSubstrError(0, "str"); + c.uselessCallsSubstrError(0, false); } std::string myName() const { diff --git a/test/teststl.cpp b/test/teststl.cpp index 413fac379..8bd5ef6e0 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -1224,28 +1224,28 @@ private: "{\n" " if (s.find(\"abc\")) { }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Suspicious checking of string::find() return value.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (performance) Inefficient usage of string::find in condition; string::compare would be faster.\n", errout.str()); // error (pointer) check("void f(const std::string *s)\n" "{\n" " if (*s.find(\"abc\")) { }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Suspicious checking of string::find() return value.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (performance) Inefficient usage of string::find in condition; string::compare would be faster.\n", errout.str()); // error (vector) check("void f(const std::vector &s)\n" "{\n" " if (s[0].find(\"abc\")) { }\n" "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Suspicious checking of string::find() return value.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (performance) Inefficient usage of string::find in condition; string::compare would be faster.\n", errout.str()); // #3162 check("void f(const std::string& s1, const std::string& s2)\n" "{\n" " if ((!s1.empty()) && (0 == s1.find(s2))) { }\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Suspicious checking of string::find() return value.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (performance) Inefficient usage of string::find in condition; string::compare would be faster.\n", errout.str()); } @@ -1783,12 +1783,14 @@ private: " s2 = s1.substr(x);\n" " s1 = s2.substr(0, x);\n" " s1 = s2.substr(0,std::string::npos);\n" + " s1 = s2.substr(x+5-n, 0);\n" " \n" "};\n"); - ASSERT_EQUALS("[test.cpp:5]: (performance) Function \'substr\' useless call. Function create copy " - "of the \'s1\' object.\n" - "[test.cpp:8]: (performance) Function \'substr\' useless call. Function create copy " - "of the \'s2\' object.\n",errout.str()); + ASSERT_EQUALS("[test.cpp:5]: (performance) Useless call of function \'substr\' because it returns a copy of " + "the object. Use operator= instead.\n" + "[test.cpp:8]: (performance) Useless call of function \'substr\' because it returns a copy of " + "the object. Use operator= instead.\n" + "[test.cpp:9]: (performance) Useless call of function \'substr\' because it returns an empty string.\n", errout.str()); check("#include \n" "int main()\n"