Fix #11166 inefficient way to remove last character from std::string (#4267)

* Fix #11166 inefficient way to remove last character from std::string

* Format

* Modify message, add test

* Format

* Fix another warning

lib/checkstl.cpp

@@ -2157,15 +2157,18 @@ void CheckStl::uselessCalls()
                 if (!var || !var->isStlType())
                     continue;
                 uselessCallsSwapError(tok, tok->str());
-            } else if (printPerformance && Token::Match(tok, "%var% . substr (") &&
+            } else if (printPerformance && Token::Match(tok, "%var% . substr (") && tok->variable() && tok->variable()->isStlStringType()) {
-                       tok->variable() && tok->variable()->isStlStringType()) {
+                const Token* funcTok = tok->tokAt(3);
-                if (Token::Match(tok->tokAt(4), "0| )")) {
+                const std::vector<const Token*> args = getArguments(funcTok);
-                    uselessCallsSubstrError(tok, false);
+                if (Token::Match(tok->tokAt(-2), "%var% =") && tok->varId() == tok->tokAt(-2)->varId() &&
-                } else if (tok->strAt(4) == "0" && tok->linkAt(3)->strAt(-1) == "npos") {
+                    !args.empty() && args[0]->hasKnownIntValue() && args[0]->getKnownIntValue() == 0) {
-                    if (!tok->linkAt(3)->previous()->variable()) // Make sure that its no variable
+                    uselessCallsSubstrError(tok, Token::simpleMatch(funcTok->astParent(), "=") ? SubstrErrorType::PREFIX : SubstrErrorType::PREFIX_CONCAT);
-                        uselessCallsSubstrError(tok, false);
+                } else if (args.empty() || (args[0]->hasKnownIntValue() && args[0]->getKnownIntValue() == 0 &&
-                } else if (Token::simpleMatch(tok->linkAt(3)->tokAt(-2), ", 0 )"))
+                                            (args.size() == 1 || (args.size() == 2 && tok->linkAt(3)->strAt(-1) == "npos" && !tok->linkAt(3)->previous()->variable())))) {
-                    uselessCallsSubstrError(tok, true);
+                    uselessCallsSubstrError(tok, SubstrErrorType::COPY);
+                } else if (args.size() == 2 && args[1]->hasKnownIntValue() && args[1]->getKnownIntValue() == 0) {
+                    uselessCallsSubstrError(tok, SubstrErrorType::EMPTY);
+                }
             } else if (printWarning && Token::Match(tok, "[{};] %var% . empty ( ) ;") &&
                        !tok->tokAt(4)->astParent() &&
                        tok->next()->variable() && tok->next()->variable()->isStlType(stl_containers_with_empty_and_clear))
@@ -2200,12 +2203,24 @@ void CheckStl::uselessCallsSwapError(const Token *tok, const std::string &varnam
                 "code is inefficient. Is the object or the parameter wrong here?", CWE628, Certainty::normal);
 }
 
-void CheckStl::uselessCallsSubstrError(const Token *tok, bool empty)
+void CheckStl::uselessCallsSubstrError(const Token *tok, SubstrErrorType type)
 {
-    if (empty)
+    std::string msg = "Ineffective call of function 'substr' because ";
-        reportError(tok, Severity::performance, "uselessCallsSubstr", "Ineffective call of function 'substr' because it returns an empty string.", CWE398, Certainty::normal);
+    switch (type) {
-    else
+    case SubstrErrorType::EMPTY:
-        reportError(tok, Severity::performance, "uselessCallsSubstr", "Ineffective call of function 'substr' because it returns a copy of the object. Use operator= instead.", CWE398, Certainty::normal);
+        msg += "it returns an empty string.";
+        break;
+    case SubstrErrorType::COPY:
+        msg += "it returns a copy of the object. Use operator= instead.";
+        break;
+    case SubstrErrorType::PREFIX:
+        msg += "a prefix of the string is assigned to itself. Use resize() or pop_back() instead.";
+        break;
+    case SubstrErrorType::PREFIX_CONCAT:
+        msg += "a prefix of the string is assigned to itself. Use replace() instead.";
+        break;
+    }
+    reportError(tok, Severity::performance, "uselessCallsSubstr", msg, CWE398, Certainty::normal);
 }
 
 void CheckStl::uselessCallsEmptyError(const Token *tok)

lib/checkstl.h

@@ -220,7 +220,8 @@ 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, bool empty);
+    enum class SubstrErrorType { EMPTY, COPY, PREFIX, PREFIX_CONCAT };
+    void uselessCallsSubstrError(const Token* tok, SubstrErrorType type);
     void uselessCallsEmptyError(const Token* tok);
     void uselessCallsRemoveError(const Token* tok, const std::string& function);
 
@@ -266,7 +267,7 @@ private:
         c.redundantIfRemoveError(nullptr);
         c.uselessCallsReturnValueError(nullptr, "str", "find");
         c.uselessCallsSwapError(nullptr, "str");
-        c.uselessCallsSubstrError(nullptr, false);
+        c.uselessCallsSubstrError(nullptr, SubstrErrorType::COPY);
         c.uselessCallsEmptyError(nullptr);
         c.uselessCallsRemoveError(nullptr, "remove");
         c.dereferenceInvalidIteratorError(nullptr, "i");

lib/checkstring.cpp

@@ -82,7 +82,7 @@ void CheckString::stringLiteralWriteError(const Token *tok, const Token *strValu
         std::string s = strValue->str();
         // 20 is an arbitrary value, the max string length shown in a warning message
         if (s.size() > 20U)
-            s = s.substr(0,17) + "..\"";
+            s.replace(17, std::string::npos, "..\"");
         errmsg += " " + s;
     }
     errmsg += " directly or indirectly is undefined behaviour.";

lib/cppcheck.cpp

@@ -316,7 +316,7 @@ static std::string executeAddon(const AddonInfo &addonInfo,
         std::string message("Failed to execute addon (command: '" + pythonExe + " " + args + "'). Exitcode is nonzero.");
         if (result.size() > 2) {
             message = message + "\n" + message + "\nOutput:\n" + result;
-            message = message.substr(0,message.find_last_not_of("\n\r"));
+            message.resize(message.find_last_not_of("\n\r"));
         }
         throw InternalError(nullptr, message);
     }

lib/errorlogger.cpp

@@ -843,7 +843,7 @@ std::string replaceStr(std::string s, const std::string &from, const std::string
             pos1++;
             continue;
         }
-        s = s.substr(0,pos1) + to + s.substr(pos2);
+        s.replace(pos1, from.size(), to);
         pos1 += to.size();
     }
     return s;

lib/importproject.cpp

@@ -139,7 +139,7 @@ static bool simplifyPathWithVariables(std::string &s, std::map<std::string, std:
             variables[var] = std::string(envValue);
             it1 = variables.find(var);
         }
-        s = s.substr(0, start) + it1->second + s.substr(end + 1);
+        s.replace(start, end - start + 1, it1->second);
     }
     if (s.find("$(") != std::string::npos)
         return false;
@@ -644,7 +644,7 @@ static void importPropertyGroup(const tinyxml2::XMLElement *node, std::map<std::
             std::string path(text);
             const std::string::size_type pos = path.find("$(IncludePath)");
             if (pos != std::string::npos)
-                path = path.substr(0,pos) + *includePath + path.substr(pos+14U);
+                path.replace(pos, 14U, *includePath);
             *includePath = path;
         }
     }

lib/token.cpp

@@ -1113,7 +1113,8 @@ Token* Token::insertToken(const std::string& tokenStr, const std::string& origin
                             nextScopeNameAddition.append(nameTok->str());
                             nextScopeNameAddition.append(" ");
                         }
-                        if (nextScopeNameAddition.length() > 0) nextScopeNameAddition = nextScopeNameAddition.substr(0, nextScopeNameAddition.length() - 1);
+                        if (!nextScopeNameAddition.empty())
+                            nextScopeNameAddition.pop_back();
                     }
                 }
 

lib/tokenize.cpp

@@ -2190,7 +2190,7 @@ namespace {
         while (!newScope1.empty()) {
             std::string::size_type separator = newScope1.rfind(" :: ", index - 1);
             if (separator != std::string::npos)
-                newScope1 = newScope1.substr(0, separator);
+                newScope1.resize(separator);
             else
                 newScope1.clear();
 
@@ -3352,8 +3352,8 @@ void Tokenizer::calculateScopes()
                     usingNamespaceName += namespaceNameToken->str();
                     usingNamespaceName += " ";
                 }
-                if (usingNamespaceName.length() > 0)
+                if (!usingNamespaceName.empty())
-                    usingNamespaceName = usingNamespaceName.substr(0, usingNamespaceName.length() - 1);
+                    usingNamespaceName.pop_back();
                 tok->scopeInfo()->usingNamespaces.insert(usingNamespaceName);
             } else if (Token::Match(tok, "namespace|class|struct|union %name% {|::|:|<")) {
                 for (Token* nameTok = tok->next(); nameTok && !Token::Match(nameTok, "{|:"); nameTok = nameTok->next()) {
@@ -3364,8 +3364,8 @@ void Tokenizer::calculateScopes()
                     nextScopeNameAddition.append(nameTok->str());
                     nextScopeNameAddition.append(" ");
                 }
-                if (nextScopeNameAddition.length() > 0)
+                if (!nextScopeNameAddition.empty())
-                    nextScopeNameAddition = nextScopeNameAddition.substr(0, nextScopeNameAddition.length() - 1);
+                    nextScopeNameAddition.pop_back();
             }
 
             if (Token::simpleMatch(tok, "{")) {
@@ -8106,7 +8106,7 @@ std::string Tokenizer::simplifyString(const std::string &source)
                 sz++;
             std::istringstream istr(str.substr(i+1, sz-1));
             istr >> std::oct >> c;
-            str = str.substr(0,i) + (char)c + str.substr(i+sz);
+            str = str.replace(i, sz, std::string(1U, (char)c));
             continue;
         }
 

test/teststl.cpp

@@ -4168,6 +4168,21 @@ private:
               "    for (;s.empty();) {}\n"
               "}");
         ASSERT_EQUALS("", errout.str());
+
+        // #11166
+        check("std::string f(std::string s) {\n"
+              "    s = s.substr(0, s.size() - 1);\n"
+              "    return s;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:2]: (performance) Ineffective call of function 'substr' because a prefix of the string is assigned to itself. Use resize() or pop_back() instead.\n",
+                      errout.str());
+
+        check("std::string f(std::string s, std::size_t start, std::size_t end, const std::string& i) {\n"
+              "    s = s.substr(0, start) + i + s.substr(end + 1);\n"
+              "    return s;\n"
+              "}\n");
+        ASSERT_EQUALS("[test.cpp:2]: (performance) Ineffective call of function 'substr' because a prefix of the string is assigned to itself. Use replace() instead.\n",
+                      errout.str());
     }
 
     void stabilityOfChecks() {