Improved checks in CheckStl:

- Improved message of stlIfStrFind according to discussion on github (77d9ed1877)
- 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

@@ -832,12 +832,12 @@ void CheckStl::if_find()
 void CheckStl::if_findError(const Token *tok, bool str)
 {
     if (str)
-        reportError(tok, Severity::warning, "stlIfStrFind",
+        reportError(tok, Severity::performance, "stlIfStrFind",
-                    "Suspicious checking of string::find() return value.\n"
+                    "Inefficient usage of string::find in condition; string::compare would be faster.\n"
-                    "Checking of string::find() return value looks Suspicious. "
+                    "Either inefficent or wrong usage of string::find. string::compare will be faster if "
-                    "string::find will return 0 if the string is found at position 0. "
+                    "string::find's result is compared with 0, because it will not scan the whole "
-                    "If that is wanted to check then string::compare is a faster alternative "
+                    "string. If your intention is to check that there are no findings in the string, "
-                    "because it doesn't scan through 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)
+        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% [,)]") &&
-            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% [,)]") &&
             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 ( )")) {
+        } else if (Token::Match(tok, ". substr (")) {
-            uselessCallsSubstrError(tok, tok->str());
+            if (Token::Match(tok->tokAt(3), "0| )"))
-        } else if (Token::Match(tok, "%var% . substr ( 0")) {
+                uselessCallsSubstrError(tok, false);
-            if (tok->strAt(5) == ")" ||
+            else if (tok->strAt(3) == "0" && tok->linkAt(2)->strAt(-1) == "npos")
-                tok->linkAt(3)->strAt(-1) == "npos")
+                uselessCallsSubstrError(tok, false);
-                uselessCallsSubstrError(tok, tok->str());
+            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;
+    if (empty)
-    errmsg << "Function \'substr\' useless call. Function create copy of the \'" << varname << "\' object.\n" <<
+        reportError(tok, Severity::performance, "uselessCallsSubstr", "Useless call of function 'substr' because it returns an empty string.");
-           "Function \'substr\' useless call. \'substr\' function create copy of the whole \'" << varname << "\' object which can decrease in performance. Please use \'=\' operator instead.";
+    else
-    reportError(tok, Severity::performance, "uselessCallsSubstr", errmsg.str());
+        reportError(tok, Severity::performance, "uselessCallsSubstr", "Useless call of function 'substr' because it returns a copy of the object. Use operator= instead.");
 }

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 {

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<std::string> &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 "
+        ASSERT_EQUALS("[test.cpp:5]: (performance) Useless call of function \'substr\' because it returns a copy of "
-                      "of the \'s1\' object.\n"
+                      "the object. Use operator= instead.\n"
-                      "[test.cpp:8]: (performance) Function \'substr\' useless call. Function create copy "
+                      "[test.cpp:8]: (performance) Useless call of function \'substr\' because it returns a copy of "
-                      "of the \'s2\' object.\n",errout.str());
+                      "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 <string>\n"
               "int main()\n"