From 091f4bcf8dd83e17ac806fa63e89ab23e66179f7 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Thu, 2 May 2019 11:04:23 +0200 Subject: [PATCH] Add check for unnecessary search before insertion This will warn for cases where searching in an associative container happens before insertion, like this: ```cpp void f1(std::set& s, unsigned x) { if (s.find(x) == s.end()) { s.insert(x); } } void f2(std::map& m, unsigned x) { if (m.find(x) == m.end()) { m.emplace(x, 1); } else { m[x] = 1; } } ``` In the case of the map it could be written as `m[x] = 1` as it will create the key if it doesnt exist, so the extra search is not necessary. I have this marked as `performance` as it is mostly concerning performance, but there could be a copy-paste error possibly, although I dont think thats common. --- cfg/boost.cfg | 6 +- cfg/cppcheck-cfg.rng | 17 +++- cfg/std.cfg | 33 +++++-- lib/checkstl.cpp | 142 +++++++++++++++++++++++++++ lib/checkstl.h | 6 ++ lib/library.cpp | 3 + lib/library.h | 2 + lib/symboldatabase.h | 8 ++ lib/token.cpp | 73 ++++++++++++++ lib/token.h | 6 ++ lib/valueflow.cpp | 30 +----- man/manual.md | 5 + test/teststl.cpp | 223 +++++++++++++++++++++++++++++++++++++++++++ 13 files changed, 510 insertions(+), 44 deletions(-) diff --git a/cfg/boost.cfg b/cfg/boost.cfg index 6801798f7..5c33f6812 100644 --- a/cfg/boost.cfg +++ b/cfg/boost.cfg @@ -33,8 +33,10 @@ - - + + + + diff --git a/cfg/cppcheck-cfg.rng b/cfg/cppcheck-cfg.rng index 93787c605..2a52b3203 100644 --- a/cfg/cppcheck-cfg.rng +++ b/cfg/cppcheck-cfg.rng @@ -337,12 +337,19 @@ - + + + std-like - + + + + std-like + + @@ -352,14 +359,16 @@ - + + + - + diff --git a/cfg/std.cfg b/cfg/std.cfg index bb765d176..84480f3d1 100644 --- a/cfg/std.cfg +++ b/cfg/std.cfg @@ -7325,8 +7325,8 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun - - + + @@ -7390,30 +7390,45 @@ The obsolete function 'gets' is called. With 'gets' you'll get a buffer overrun - + + - + - - + + - - - + + + + + + + + + + + + + + + + + diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 80ee8a518..2a1f854a7 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -1237,6 +1237,148 @@ void CheckStl::if_findError(const Token *tok, bool str) reportError(tok, Severity::warning, "stlIfFind", "Suspicious condition. The result of find() is an iterator, but it is not properly checked.", CWE398, false); } +static std::pair isMapFind(const Token *tok) +{ + if (!Token::simpleMatch(tok, "(")) + return {}; + if (!Token::simpleMatch(tok->astOperand1(), ".")) + return {}; + if (!astIsContainer(tok->astOperand1()->astOperand1())) + return {}; + const Token * contTok = tok->astOperand1()->astOperand1(); + const Library::Container * container = contTok->valueType()->container; + if (!container) + return {}; + if (!container->stdAssociativeLike) + return {}; + if (!Token::Match(tok->astOperand1(), ". find|count (")) + return {}; + if (!tok->astOperand2()) + return {}; + return {contTok, tok->astOperand2()}; +} + +static const Token *skipLocalVars(const Token *tok) +{ + if (!tok) + return tok; + if (Token::simpleMatch(tok, "{")) + return skipLocalVars(tok->next()); + const Scope *scope = tok->scope(); + + const Token *top = tok->astTop(); + if (!top) { + const Token *semi = Token::findsimplematch(tok, ";"); + if (!semi) + return tok; + if (!Token::Match(semi->previous(), "%var% ;")) + return tok; + const Token *varTok = semi->previous(); + const Variable *var = varTok->variable(); + if (!var) + return tok; + if (var->nameToken() != varTok) + return tok; + return skipLocalVars(semi->next()); + } + if (Token::Match(top, "%assign%")) { + const Token *varTok = top->astOperand1(); + if (!Token::Match(varTok, "%var%")) + return tok; + const Variable *var = varTok->variable(); + if (!var) + return tok; + if (var->scope() != scope) + return tok; + const Token *endTok = nextAfterAstRightmostLeaf(top); + if (!endTok) + return tok; + return skipLocalVars(endTok->next()); + } + return tok; +} + +static const Token *findInsertValue(const Token *tok, const Token *containerTok, const Token *keyTok, const Library &library) +{ + const Token *startTok = skipLocalVars(tok); + const Token *top = startTok->astTop(); + + const Token *icontainerTok = nullptr; + const Token *ikeyTok = nullptr; + const Token *ivalueTok = nullptr; + if (Token::simpleMatch(top, "=") && Token::simpleMatch(top->astOperand1(), "[")) { + icontainerTok = top->astOperand1()->astOperand1(); + ikeyTok = top->astOperand1()->astOperand2(); + ivalueTok = top->astOperand2(); + } + if (Token::simpleMatch(top, "(") && Token::Match(top->astOperand1(), ". insert|emplace (") && !astIsIterator(top->astOperand1()->tokAt(2))) { + icontainerTok = top->astOperand1()->astOperand1(); + const Token *itok = top->astOperand1()->tokAt(2)->astOperand2(); + if (Token::simpleMatch(itok, ",")) { + ikeyTok = itok->astOperand1(); + ivalueTok = itok->astOperand2(); + } else { + ikeyTok = itok; + } + } + if (!ikeyTok || !icontainerTok) + return nullptr; + if (isSameExpression(true, true, containerTok, icontainerTok, library, true, false) && + isSameExpression(true, true, keyTok, ikeyTok, library, true, true)) { + if (ivalueTok) + return ivalueTok; + else + return ikeyTok; + } + return nullptr; +} + +void CheckStl::checkFindInsert() +{ + if (!mSettings->isEnabled(Settings::PERFORMANCE)) + return; + + const SymbolDatabase *const symbolDatabase = mTokenizer->getSymbolDatabase(); + for (const Scope *scope : symbolDatabase->functionScopes) { + for (const Token *tok = scope->bodyStart->next(); tok != scope->bodyEnd; tok = tok->next()) { + if (!Token::simpleMatch(tok, "if (")) + continue; + if (!Token::simpleMatch(tok->next()->link(), ") {")) + continue; + if (!Token::Match(tok->next()->astOperand2(), "%comp%")) + continue; + const Token *condTok = tok->next()->astOperand2(); + const Token *containerTok; + const Token *keyTok; + std::tie(containerTok, keyTok) = isMapFind(condTok->astOperand1()); + if (!containerTok) + continue; + + const Token *thenTok = tok->next()->link()->next(); + const Token *valueTok = findInsertValue(thenTok, containerTok, keyTok, mSettings->library); + if (!valueTok) + continue; + + if (Token::simpleMatch(thenTok->link(), "} else {")) { + const Token *valueTok2 = + findInsertValue(thenTok->link()->tokAt(2), containerTok, keyTok, mSettings->library); + if (!valueTok2) + continue; + if (isSameExpression(true, true, valueTok, valueTok2, mSettings->library, true, true)) { + checkFindInsertError(valueTok); + } + } else { + checkFindInsertError(valueTok); + } + } + } +} + +void CheckStl::checkFindInsertError(const Token *tok) +{ + reportError( + tok, Severity::performance, "stlFindInsert", "Searching before insertion is not necessary.", CWE398, false); +} /** * Is container.size() slow? diff --git a/lib/checkstl.h b/lib/checkstl.h index a3315341b..4869a6330 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -62,6 +62,7 @@ public: CheckStl checkStl(tokenizer, settings, errorLogger); checkStl.erase(); checkStl.if_find(); + checkStl.checkFindInsert(); checkStl.iterators(); checkStl.mismatchingContainers(); checkStl.missingComparison(); @@ -132,6 +133,8 @@ public: /** if (a.find(x)) - possibly incorrect condition */ void if_find(); + void checkFindInsert(); + /** * Suggest using empty() instead of checking size() against zero for containers. * Item 4 from Scott Meyers book "Effective STL". @@ -202,6 +205,7 @@ private: void invalidPointerError(const Token* tok, const std::string& func, const std::string& pointer_name); void stlBoundariesError(const Token* tok); void if_findError(const Token* tok, bool str); + void checkFindInsertError(const Token *tok); void sizeError(const Token* tok); void redundantIfRemoveError(const Token* tok); @@ -239,6 +243,7 @@ private: c.stlBoundariesError(nullptr); c.if_findError(nullptr, false); c.if_findError(nullptr, true); + c.checkFindInsertError(nullptr); c.string_c_strError(nullptr); c.string_c_strReturn(nullptr); c.string_c_strParam(nullptr, 0); @@ -270,6 +275,7 @@ private: "- for vectors: using iterator/pointer after push_back has been used\n" "- optimisation: use empty() instead of size() to guarantee fast code\n" "- suspicious condition when using find\n" + "- unnecessary searching in associative containers\n" "- redundant condition\n" "- common mistakes when using string::c_str()\n" "- useless calls of string and STL functions\n" diff --git a/lib/library.cpp b/lib/library.cpp index d49efdf89..affcb26cf 100644 --- a/lib/library.cpp +++ b/lib/library.cpp @@ -483,6 +483,9 @@ Library::Error Library::load(const tinyxml2::XMLDocument &doc) const char* const string = containerNode->Attribute("string"); if (string) container.stdStringLike = std::string(string) == "std-like"; + const char* const associative = containerNode->Attribute("associative"); + if (associative) + container.stdAssociativeLike = std::string(associative) == "std-like"; } else unknown_elements.insert(containerNodeName); } diff --git a/lib/library.h b/lib/library.h index cfe5f87ed..d47c82700 100644 --- a/lib/library.h +++ b/lib/library.h @@ -181,6 +181,7 @@ public: size_templateArgNo(-1), arrayLike_indexOp(false), stdStringLike(false), + stdAssociativeLike(false), opLessAllowed(true) { } @@ -202,6 +203,7 @@ public: int size_templateArgNo; bool arrayLike_indexOp; bool stdStringLike; + bool stdAssociativeLike; bool opLessAllowed; Action getAction(const std::string& function) const { diff --git a/lib/symboldatabase.h b/lib/symboldatabase.h index ea60721cc..b4941afc4 100644 --- a/lib/symboldatabase.h +++ b/lib/symboldatabase.h @@ -852,6 +852,14 @@ public: static bool returnsReference(const Function *function); + const Token* returnDefEnd() const { + if (this->hasTrailingReturnType()) { + return Token::findsimplematch(retDef, "{"); + } else { + return tokenDef; + } + } + /** * @return token to ":" if the function is a constructor * and it contains member initialization otherwise a nullptr is returned diff --git a/lib/token.cpp b/lib/token.cpp index 8c2d18e51..ac91ffee6 100644 --- a/lib/token.cpp +++ b/lib/token.cpp @@ -1759,6 +1759,79 @@ void Token::type(const ::Type *t) tokType(eName); } +const ::Type *Token::typeOf(const Token *tok) +{ + if (Token::simpleMatch(tok, "return")) { + const Scope *scope = tok->scope(); + if (!scope) + return nullptr; + const Function *function = scope->function; + if (!function) + return nullptr; + return function->retType; + } else if (Token::Match(tok, "%type%")) { + return tok->type(); + } else if (Token::Match(tok, "%var%")) { + const Variable *var = tok->variable(); + if (!var) + return nullptr; + return var->type(); + } else if (Token::Match(tok, "%name%")) { + const Function *function = tok->function(); + if (!function) + return nullptr; + return function->retType; + } else if (Token::simpleMatch(tok, "=")) { + return Token::typeOf(tok->astOperand1()); + } else if (Token::simpleMatch(tok, ".")) { + return Token::typeOf(tok->astOperand2()); + } + return nullptr; +} + +std::pair Token::typeDecl(const Token * tok) +{ + if (Token::simpleMatch(tok, "return")) { + const Scope *scope = tok->scope(); + if (!scope) + return {}; + const Function *function = scope->function; + if (!function) + return {}; + return {function->retDef, function->returnDefEnd()}; + } else if (Token::Match(tok, "%type%")) { + return {tok, tok->next()}; + } else if (Token::Match(tok, "%var%")) { + const Variable *var = tok->variable(); + if (!var) + return {}; + if (!var->typeStartToken() || !var->typeEndToken()) + return {}; + return {var->typeStartToken(), var->typeEndToken()->next()}; + } else if (Token::Match(tok, "%name%")) { + const Function *function = tok->function(); + if (!function) + return {}; + return {function->retDef, function->returnDefEnd()}; + } else if (Token::simpleMatch(tok, "=")) { + return Token::typeDecl(tok->astOperand1()); + } else if (Token::simpleMatch(tok, ".")) { + return Token::typeDecl(tok->astOperand2()); + } else { + const ::Type * t = typeOf(tok); + if (!t || !t->classDef) + return {}; + return {t->classDef->next(), t->classDef->tokAt(2)}; + } +} +std::string Token::typeStr(const Token* tok) +{ + std::pair r = Token::typeDecl(tok); + if (!r.first || !r.second) + return ""; + return r.first->stringifyList(r.second, false); +} + TokenImpl::~TokenImpl() { delete mOriginalName; diff --git a/lib/token.h b/lib/token.h index 8cc82b987..ebbf11bae 100644 --- a/lib/token.h +++ b/lib/token.h @@ -806,6 +806,12 @@ public: return mTokType == eType ? mImpl->mType : nullptr; } + static const ::Type *typeOf(const Token *tok); + + static std::pair typeDecl(const Token * tok); + + static std::string typeStr(const Token* tok); + /** * @return a pointer to the Enumerator associated with this token. */ diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index bdda39a00..15ab012fb 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -3142,39 +3142,11 @@ static void valueFlowLifetimeFunction(Token *tok, TokenList *tokenlist, ErrorLog } } -static const Type *getTypeOf(const Token *tok) -{ - if (Token::simpleMatch(tok, "return")) { - const Scope *scope = tok->scope(); - if (!scope) - return nullptr; - const Function *function = scope->function; - if (!function) - return nullptr; - return function->retType; - } else if (Token::Match(tok, "%type%")) { - return tok->type(); - } else if (Token::Match(tok, "%var%")) { - const Variable *var = tok->variable(); - if (!var) - return nullptr; - return var->type(); - } else if (Token::Match(tok, "%name%")) { - const Function *function = tok->function(); - if (!function) - return nullptr; - return function->retType; - } else if (Token::simpleMatch(tok, "=")) { - return getTypeOf(tok->astOperand1()); - } - return nullptr; -} - static void valueFlowLifetimeConstructor(Token *tok, TokenList *tokenlist, ErrorLogger *errorLogger, const Settings *settings) { if (!Token::Match(tok, "(|{")) return; - if (const Type *t = getTypeOf(tok->previous())) { + if (const Type *t = Token::typeOf(tok->previous())) { const Scope *scope = t->classScope; if (!scope) return; diff --git a/man/manual.md b/man/manual.md index b07d05410..a606eef7d 100644 --- a/man/manual.md +++ b/man/manual.md @@ -1330,6 +1330,11 @@ The following example provides a definition for std::vector, based on the defini +The tag `` can be added as well to provide more information about the type of container. Here is some of the attributes that can be set: + +* `string='std-like'` can be set for containers that match `std::string` interfaces. +* `associative='std-like'` can be set for containers that match C++'s `AssociativeContainer` interfaces. + ## HTML Report You can convert the XML output from cppcheck into a HTML report. You'll need Python and the pygments module ( for this to work. In the Cppcheck source tree there is a folder htmlreport that contains a script that transforms a Cppcheck XML file into HTML output. diff --git a/test/teststl.cpp b/test/teststl.cpp index a5a258b91..2c8fe304a 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -156,6 +156,7 @@ private: TEST_CASE(loopAlgoIncrement); TEST_CASE(loopAlgoConditional); TEST_CASE(loopAlgoMinMax); + TEST_CASE(findInsert); } void check(const char code[], const bool inconclusive=false, const Standards::cppstd_t cppstandard=Standards::CPP11) { @@ -3844,6 +3845,228 @@ private: true); ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::accumulate algorithm instead of a raw loop.\n", errout.str()); } + + void findInsert() { + check("void f1(std::set& s, unsigned x) {\n" + " if (s.find(x) == s.end()) {\n" + " s.insert(x);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void f2(std::map& m, unsigned x) {\n" + " if (m.find(x) == m.end()) {\n" + " m.emplace(x, 1);\n" + " } else {\n" + " m[x] = 1;\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void f3(std::map& m, unsigned x) {\n" + " if (m.count(x) == 0) {\n" + " m.emplace(x, 1);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void f4(std::set& s, unsigned x) {\n" + " if (s.find(x) == s.end()) {\n" + " s.insert(x);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void f5(std::map& m, unsigned x) {\n" + " if (m.count(x) == 0) {\n" + " m.emplace(x, 1);\n" + " } else {\n" + " m[x] = 1;\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void f6(std::map& m, unsigned x) {\n" + " if (m.count(x) == 0) {\n" + " m.emplace(x, 1);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void f1(std::unordered_set& s, unsigned x) {\n" + " if (s.find(x) == s.end()) {\n" + " s.insert(x);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void f2(std::unordered_map& m, unsigned x) {\n" + " if (m.find(x) == m.end()) {\n" + " m.emplace(x, 1);\n" + " } else {\n" + " m[x] = 1;\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void f3(std::unordered_map& m, unsigned x) {\n" + " if (m.count(x) == 0) {\n" + " m.emplace(x, 1);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void f4(std::unordered_set& s, unsigned x) {\n" + " if (s.find(x) == s.end()) {\n" + " s.insert(x);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void f5(std::unordered_map& m, unsigned x) {\n" + " if (m.count(x) == 0) {\n" + " m.emplace(x, 1);\n" + " } else {\n" + " m[x] = 1;\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void f6(std::unordered_map& m, unsigned x) {\n" + " if (m.count(x) == 0) {\n" + " m.emplace(x, 1);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (performance) Searching before insertion is not necessary.\n", errout.str()); + + check("void g1(std::map& m, unsigned x) {\n" + " if (m.find(x) == m.end()) {\n" + " m.emplace(x, 1);\n" + " } else {\n" + " m[x] = 2;\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void g1(std::map& m, unsigned x) {\n" + " if (m.count(x) == 0) {\n" + " m.emplace(x, 1);\n" + " } else {\n" + " m[x] = 2;\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f1(QSet& s, unsigned x) {\n" + " if (s.find(x) == s.end()) {\n" + " s.insert(x);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f1(std::multiset& s, unsigned x) {\n" + " if (s.find(x) == s.end()) {\n" + " s.insert(x);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f2(std::multimap& m, unsigned x) {\n" + " if (m.find(x) == m.end()) {\n" + " m.emplace(x, 1);\n" + " } else {\n" + " m[x] = 1;\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f3(std::multimap& m, unsigned x) {\n" + " if (m.count(x) == 0) {\n" + " m.emplace(x, 1);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f4(std::multiset& s, unsigned x) {\n" + " if (s.find(x) == s.end()) {\n" + " s.insert(x);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f5(std::multimap& m, unsigned x) {\n" + " if (m.count(x) == 0) {\n" + " m.emplace(x, 1);\n" + " } else {\n" + " m[x] = 1;\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f1(std::unordered_multiset& s, unsigned x) {\n" + " if (s.find(x) == s.end()) {\n" + " s.insert(x);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f2(std::unordered_multimap& m, unsigned x) {\n" + " if (m.find(x) == m.end()) {\n" + " m.emplace(x, 1);\n" + " } else {\n" + " m[x] = 1;\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f3(std::unordered_multimap& m, unsigned x) {\n" + " if (m.count(x) == 0) {\n" + " m.emplace(x, 1);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f4(std::unordered_multiset& s, unsigned x) {\n" + " if (s.find(x) == s.end()) {\n" + " s.insert(x);\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("void f5(std::unordered_multimap& m, unsigned x) {\n" + " if (m.count(x) == 0) {\n" + " m.emplace(x, 1);\n" + " } else {\n" + " m[x] = 1;\n" + " }\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + } }; REGISTER_TEST(TestStl)