From 47c2a01392e9fdf5d6e60ebaaa3dba447ae4ea75 Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Mon, 11 Jul 2022 23:07:37 +0200 Subject: [PATCH] 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 | 43 +++++++++++++++++++++++++++++-------------- lib/checkstl.h | 5 +++-- lib/checkstring.cpp | 2 +- lib/cppcheck.cpp | 2 +- lib/errorlogger.cpp | 2 +- lib/importproject.cpp | 4 ++-- lib/token.cpp | 3 ++- lib/tokenize.cpp | 12 ++++++------ test/teststl.cpp | 15 +++++++++++++++ 9 files changed, 60 insertions(+), 28 deletions(-) diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 40a6469cb..a7ed835c0 100644 --- a/lib/checkstl.cpp +++ b/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 (") && - tok->variable() && tok->variable()->isStlStringType()) { - if (Token::Match(tok->tokAt(4), "0| )")) { - uselessCallsSubstrError(tok, false); - } else if (tok->strAt(4) == "0" && tok->linkAt(3)->strAt(-1) == "npos") { - if (!tok->linkAt(3)->previous()->variable()) // Make sure that its no variable - uselessCallsSubstrError(tok, false); - } else if (Token::simpleMatch(tok->linkAt(3)->tokAt(-2), ", 0 )")) - uselessCallsSubstrError(tok, true); + } else if (printPerformance && Token::Match(tok, "%var% . substr (") && tok->variable() && tok->variable()->isStlStringType()) { + const Token* funcTok = tok->tokAt(3); + const std::vector args = getArguments(funcTok); + if (Token::Match(tok->tokAt(-2), "%var% =") && tok->varId() == tok->tokAt(-2)->varId() && + !args.empty() && args[0]->hasKnownIntValue() && args[0]->getKnownIntValue() == 0) { + uselessCallsSubstrError(tok, Token::simpleMatch(funcTok->astParent(), "=") ? SubstrErrorType::PREFIX : SubstrErrorType::PREFIX_CONCAT); + } else if (args.empty() || (args[0]->hasKnownIntValue() && args[0]->getKnownIntValue() == 0 && + (args.size() == 1 || (args.size() == 2 && tok->linkAt(3)->strAt(-1) == "npos" && !tok->linkAt(3)->previous()->variable())))) { + 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) - reportError(tok, Severity::performance, "uselessCallsSubstr", "Ineffective call of function 'substr' because it returns an empty string.", CWE398, Certainty::normal); - else - reportError(tok, Severity::performance, "uselessCallsSubstr", "Ineffective call of function 'substr' because it returns a copy of the object. Use operator= instead.", CWE398, Certainty::normal); + std::string msg = "Ineffective call of function 'substr' because "; + switch (type) { + case SubstrErrorType::EMPTY: + 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) diff --git a/lib/checkstl.h b/lib/checkstl.h index 3c0e2dc90..dd450fb2e 100644 --- a/lib/checkstl.h +++ b/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"); diff --git a/lib/checkstring.cpp b/lib/checkstring.cpp index 12a3cc25c..cd6617abb 100644 --- a/lib/checkstring.cpp +++ b/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."; diff --git a/lib/cppcheck.cpp b/lib/cppcheck.cpp index cdacab5be..49c0a877b 100644 --- a/lib/cppcheck.cpp +++ b/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); } diff --git a/lib/errorlogger.cpp b/lib/errorlogger.cpp index 180929b2e..d8ed08ea2 100644 --- a/lib/errorlogger.cpp +++ b/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; diff --git a/lib/importproject.cpp b/lib/importproject.cpp index ad927a59e..f0a67693f 100644 --- a/lib/importproject.cpp +++ b/lib/importproject.cpp @@ -139,7 +139,7 @@ static bool simplifyPathWithVariables(std::string &s, std::mapsecond + 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::mapstr()); nextScopeNameAddition.append(" "); } - if (nextScopeNameAddition.length() > 0) nextScopeNameAddition = nextScopeNameAddition.substr(0, nextScopeNameAddition.length() - 1); + if (!nextScopeNameAddition.empty()) + nextScopeNameAddition.pop_back(); } } diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 8f3003a6d..2d79460d7 100644 --- a/lib/tokenize.cpp +++ b/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) - usingNamespaceName = usingNamespaceName.substr(0, usingNamespaceName.length() - 1); + if (!usingNamespaceName.empty()) + 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) - nextScopeNameAddition = nextScopeNameAddition.substr(0, nextScopeNameAddition.length() - 1); + if (!nextScopeNameAddition.empty()) + 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; } diff --git a/test/teststl.cpp b/test/teststl.cpp index 31fd0d54e..8656824ba 100644 --- a/test/teststl.cpp +++ b/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() {