diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 3de8e481b..c1f3357a0 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -2501,44 +2501,77 @@ void CheckStl::useStlAlgorithm() } } -void CheckStl::knownEmptyContainerLoopError(const Token *tok) +void CheckStl::knownEmptyContainerError(const Token *tok, const std::string& algo) { - const std::string cont = tok ? tok->expressionString() : std::string("var"); + const std::string var = tok ? tok->expressionString() : std::string("var"); + + std::string msg; + if (astIsIterator(tok)) { + msg = "Using " + algo + " with iterator '" + var + "' that is always empty."; + } else { + msg = "Iterating over container '" + var + "' that is always empty."; + } reportError(tok, Severity::style, - "knownEmptyContainerLoop", - "Iterating over container '" + cont + "' that is always empty.", CWE398, false); + "knownEmptyContainer", + msg, CWE398, false); } -void CheckStl::knownEmptyContainerLoop() +static bool isKnownEmptyContainer(const Token* tok) +{ + if (!tok) + return false; + for (const ValueFlow::Value& v:tok->values()) { + if (!v.isKnown()) + continue; + if (!v.isContainerSizeValue()) + continue; + if (v.intvalue != 0) + continue; + return true; + } + return false; +} + +void CheckStl::knownEmptyContainer() { if (!mSettings->isEnabled(Settings::STYLE)) return; for (const Scope *function : mTokenizer->getSymbolDatabase()->functionScopes) { for (const Token *tok = function->bodyStart; tok != function->bodyEnd; tok = tok->next()) { + + if (!Token::Match(tok, "%name% ( !!)")) + continue; + // Parse range-based for loop - if (!Token::simpleMatch(tok, "for (")) - continue; - if (!Token::simpleMatch(tok->next()->link(), ") {")) - continue; - const Token *bodyTok = tok->next()->link()->next(); - const Token *splitTok = tok->next()->astOperand2(); - if (!Token::simpleMatch(splitTok, ":")) - continue; - const Token* contTok = splitTok->astOperand2(); - if (!contTok) - continue; - for (const ValueFlow::Value& v:contTok->values()) { - if (!v.isKnown()) + if (tok->str() == "for") { + if (!Token::simpleMatch(tok->next()->link(), ") {")) continue; - if (!v.isContainerSizeValue()) + const Token *bodyTok = tok->next()->link()->next(); + const Token *splitTok = tok->next()->astOperand2(); + if (!Token::simpleMatch(splitTok, ":")) continue; - if (v.intvalue != 0) + const Token* contTok = splitTok->astOperand2(); + if (!isKnownEmptyContainer(contTok)) continue; - knownEmptyContainerLoopError(contTok); + knownEmptyContainerError(contTok, ""); + } else { + const std::vector args = getArguments(tok); + if (args.empty()) + continue; + + for (int argnr = 1; argnr <= args.size(); ++argnr) { + const Library::ArgumentChecks::IteratorInfo *i = mSettings->library.getArgIteratorInfo(tok, argnr); + if (!i) + continue; + const Token * const argTok = args[argnr - 1]; + if (!isKnownEmptyContainer(argTok)) + continue; + knownEmptyContainerError(argTok, tok->str()); + break; + + } } - - } } } diff --git a/lib/checkstl.h b/lib/checkstl.h index 6b9e5606c..3168d1b27 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -81,7 +81,7 @@ public: checkStl.invalidContainerLoop(); checkStl.mismatchingContainers(); checkStl.mismatchingContainerIterator(); - checkStl.knownEmptyContainerLoop(); + checkStl.knownEmptyContainer(); checkStl.stlBoundaries(); checkStl.checkDereferenceInvalidIterator(); @@ -190,7 +190,7 @@ public: /** @brief Look for loops that can replaced with std algorithms */ void useStlAlgorithm(); - void knownEmptyContainerLoop(); + void knownEmptyContainer(); void checkMutexes(); @@ -238,7 +238,7 @@ private: void useStlAlgorithmError(const Token *tok, const std::string &algoName); - void knownEmptyContainerLoopError(const Token *tok); + void knownEmptyContainerError(const Token *tok, const std::string& algo); void globalLockGuardError(const Token *tok); void localMutexError(const Token *tok); @@ -279,7 +279,7 @@ private: c.dereferenceInvalidIteratorError(nullptr, "i"); c.readingEmptyStlContainerError(nullptr); c.useStlAlgorithmError(nullptr, ""); - c.knownEmptyContainerLoopError(nullptr); + c.knownEmptyContainerError(nullptr, ""); c.globalLockGuardError(nullptr); c.localMutexError(nullptr); } diff --git a/test/teststl.cpp b/test/teststl.cpp index 876da2e63..9ef75a799 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -167,7 +167,7 @@ private: TEST_CASE(invalidContainerLoop); TEST_CASE(findInsert); - TEST_CASE(checkKnownEmptyContainerLoop); + TEST_CASE(checkKnownEmptyContainer); TEST_CASE(checkMutexes); } @@ -547,13 +547,11 @@ private: } void iterator5() { - check("void foo()\n" + check("void foo(std::vector ints1, std::vector ints2)\n" "{\n" - " std::vector ints1;\n" - " std::vector ints2;\n" " std::vector::iterator it = std::find(ints1.begin(), ints2.end(), 22);\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Iterators of different containers 'ints1' and 'ints2' are used together.\n", + ASSERT_EQUALS("[test.cpp:3]: (error) Iterators of different containers 'ints1' and 'ints2' are used together.\n", errout.str()); } @@ -579,56 +577,44 @@ private: } void iterator7() { - check("void foo()\n" + check("void foo(std::vector ints1, std::vector ints2)\n" "{\n" - " std::vector ints1;\n" - " std::vector ints2;\n" " std::vector::iterator it = std::inplace_merge(ints1.begin(), std::advance(ints1.rbegin(), 5), ints2.end());\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Iterators of different containers 'ints1' and 'ints2' are used together.\n", + ASSERT_EQUALS("[test.cpp:3]: (error) Iterators of different containers 'ints1' and 'ints2' are used together.\n", errout.str()); - check("void foo()\n" + check("void foo(std::vector ints1, std::vector ints2)\n" "{\n" - " std::vector ints1;\n" - " std::vector ints2;\n" " std::vector::iterator it = std::inplace_merge(ints1.begin(), std::advance(ints2.rbegin(), 5), ints1.end());\n" "}"); ASSERT_EQUALS("", errout.str()); } void iterator8() { - check("void foo()\n" + check("void foo(std::vector ints1, std::vector ints2)\n" "{\n" - " std::vector ints1;\n" - " std::vector ints2;\n" " std::vector::iterator it = std::find_first_of(ints1.begin(), ints2.end(), ints1.begin(), ints1.end());\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Iterators of different containers 'ints1' and 'ints2' are used together.\n", + ASSERT_EQUALS("[test.cpp:3]: (error) Iterators of different containers 'ints1' and 'ints2' are used together.\n", errout.str()); - check("void foo()\n" + check("void foo(std::vector ints1, std::vector ints2)\n" "{\n" - " std::vector ints1;\n" - " std::vector ints2;\n" " std::vector::iterator it = std::find_first_of(ints1.begin(), ints1.end(), ints2.begin(), ints1.end());\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Iterators of different containers 'ints2' and 'ints1' are used together.\n", + ASSERT_EQUALS("[test.cpp:3]: (error) Iterators of different containers 'ints2' and 'ints1' are used together.\n", errout.str()); - check("void foo()\n" + check("void foo(std::vector ints1, std::vector ints2)\n" "{\n" - " std::vector ints1;\n" - " std::vector ints2;\n" " std::vector::iterator it = std::find_first_of(foo.bar.begin(), foo.bar.end()-6, ints2.begin(), ints1.end());\n" "}"); - ASSERT_EQUALS("[test.cpp:5]: (error) Iterators of different containers 'ints2' and 'ints1' are used together.\n", + ASSERT_EQUALS("[test.cpp:3]: (error) Iterators of different containers 'ints2' and 'ints1' are used together.\n", errout.str()); - check("void foo()\n" + check("void foo(std::vector ints1, std::vector ints2)\n" "{\n" - " std::vector ints1;\n" - " std::vector ints2;\n" " std::vector::iterator it = std::find_first_of(ints1.begin(), ints1.end(), ints2.begin(), ints2.end());\n" "}"); ASSERT_EQUALS("", errout.str()); @@ -3397,8 +3383,7 @@ private: "}"); ASSERT_EQUALS("", errout.str()); - check("void f() {\n" - " std::vector a;\n" + check("void f(std::vector a) {\n" " std::remove(a.begin(), a.end(), val);\n" " std::remove_if(a.begin(), a.end(), val);\n" " std::unique(a.begin(), a.end(), val);\n" @@ -3406,9 +3391,9 @@ private: " a.erase(std::remove(a.begin(), a.end(), val));\n" " std::remove(\"foo.txt\");\n" "}"); - ASSERT_EQUALS("[test.cpp:3]: (warning) Return value of std::remove() ignored. Elements remain in container.\n" - "[test.cpp:4]: (warning) Return value of std::remove_if() ignored. Elements remain in container.\n" - "[test.cpp:5]: (warning) Return value of std::unique() ignored. Elements remain in container.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (warning) Return value of std::remove() ignored. Elements remain in container.\n" + "[test.cpp:3]: (warning) Return value of std::remove_if() ignored. Elements remain in container.\n" + "[test.cpp:4]: (warning) Return value of std::unique() ignored. Elements remain in container.\n", errout.str()); // #4431 - fp check("bool f() {\n" @@ -4648,7 +4633,7 @@ private: ASSERT_EQUALS("", errout.str()); } - void checkKnownEmptyContainerLoop() { + void checkKnownEmptyContainer() { check("void f() {\n" " std::vector v;\n" " for(auto x:v) {}\n" @@ -4676,6 +4661,20 @@ private: "}\n", true); ASSERT_EQUALS("", errout.str()); + + check("void f() {\n" + " std::vector v;\n" + " std::sort(v.begin(), v.end());\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:3]: (style) Using sort with iterator 'v.begin()' that is always empty.\n", errout.str()); + + check("void f() {\n" + " std::vector v;\n" + " v.insert(v.end(), 1);\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); } void checkMutexes() {