walkero
/
cppcheck
1
0
Fork 0
Code Issues Pull Requests Projects Releases 1 Wiki Activity

Improve check: check for known empty containers passed to algorithms (#2768)

This commit is contained in:
Paul Fultz II 2020-09-02 00:11:23 -05:00 committed by GitHub
parent 8e79b0c8bc
commit dea5a23c34
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 92 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

79
lib/checkstl.cpp
View File

@ -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, reportError(tok, Severity::style,
"knownEmptyContainerLoop", "knownEmptyContainer",
"Iterating over container '" + cont + "' that is always empty.", CWE398, false); 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)) if (!mSettings->isEnabled(Settings::STYLE))
return; return;
for (const Scope *function : mTokenizer->getSymbolDatabase()->functionScopes) { for (const Scope *function : mTokenizer->getSymbolDatabase()->functionScopes) {
for (const Token *tok = function->bodyStart; tok != function->bodyEnd; tok = tok->next()) { for (const Token *tok = function->bodyStart; tok != function->bodyEnd; tok = tok->next()) {
if (!Token::Match(tok, "%name% ( !!)"))
continue;
// Parse range-based for loop // Parse range-based for loop
if (!Token::simpleMatch(tok, "for (")) if (tok->str() == "for") {
continue; if (!Token::simpleMatch(tok->next()->link(), ") {"))
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())
continue; continue;
if (!v.isContainerSizeValue()) const Token *bodyTok = tok->next()->link()->next();
const Token *splitTok = tok->next()->astOperand2();
if (!Token::simpleMatch(splitTok, ":"))
continue; continue;
if (v.intvalue != 0) const Token* contTok = splitTok->astOperand2();
if (!isKnownEmptyContainer(contTok))
continue; continue;
knownEmptyContainerLoopError(contTok); knownEmptyContainerError(contTok, "");
} else {
const std::vector<const Token *> 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;
}
} }
} }
} }
} }

8
lib/checkstl.h
View File

@ -81,7 +81,7 @@ public:
checkStl.invalidContainerLoop(); checkStl.invalidContainerLoop();
checkStl.mismatchingContainers(); checkStl.mismatchingContainers();
checkStl.mismatchingContainerIterator(); checkStl.mismatchingContainerIterator();
checkStl.knownEmptyContainerLoop(); checkStl.knownEmptyContainer();
checkStl.stlBoundaries(); checkStl.stlBoundaries();
checkStl.checkDereferenceInvalidIterator(); checkStl.checkDereferenceInvalidIterator();
@ -190,7 +190,7 @@ public:
/** @brief Look for loops that can replaced with std algorithms */ /** @brief Look for loops that can replaced with std algorithms */
void useStlAlgorithm(); void useStlAlgorithm();
void knownEmptyContainerLoop(); void knownEmptyContainer();
void checkMutexes(); void checkMutexes();
@ -238,7 +238,7 @@ private:
void useStlAlgorithmError(const Token *tok, const std::string &algoName); 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 globalLockGuardError(const Token *tok);
void localMutexError(const Token *tok); void localMutexError(const Token *tok);
@ -279,7 +279,7 @@ private:
c.dereferenceInvalidIteratorError(nullptr, "i"); c.dereferenceInvalidIteratorError(nullptr, "i");
c.readingEmptyStlContainerError(nullptr); c.readingEmptyStlContainerError(nullptr);
c.useStlAlgorithmError(nullptr, ""); c.useStlAlgorithmError(nullptr, "");
c.knownEmptyContainerLoopError(nullptr); c.knownEmptyContainerError(nullptr, "");
c.globalLockGuardError(nullptr); c.globalLockGuardError(nullptr);
c.localMutexError(nullptr); c.localMutexError(nullptr);
} }

65
test/teststl.cpp
View File

@ -167,7 +167,7 @@ private:
TEST_CASE(invalidContainerLoop); TEST_CASE(invalidContainerLoop);
TEST_CASE(findInsert); TEST_CASE(findInsert);
TEST_CASE(checkKnownEmptyContainerLoop); TEST_CASE(checkKnownEmptyContainer);
TEST_CASE(checkMutexes); TEST_CASE(checkMutexes);
} }
@ -547,13 +547,11 @@ private:
} }
void iterator5() { void iterator5() {
check("void foo()\n" check("void foo(std::vector<int> ints1, std::vector<int> ints2)\n"
"{\n" "{\n"
" std::vector<int> ints1;\n"
" std::vector<int> ints2;\n"
" std::vector<int>::iterator it = std::find(ints1.begin(), ints2.end(), 22);\n" " std::vector<int>::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()); errout.str());
} }
@ -579,56 +577,44 @@ private:
} }
void iterator7() { void iterator7() {
check("void foo()\n" check("void foo(std::vector<int> ints1, std::vector<int> ints2)\n"
"{\n" "{\n"
" std::vector<int> ints1;\n"
" std::vector<int> ints2;\n"
" std::vector<int>::iterator it = std::inplace_merge(ints1.begin(), std::advance(ints1.rbegin(), 5), ints2.end());\n" " std::vector<int>::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()); errout.str());
check("void foo()\n" check("void foo(std::vector<int> ints1, std::vector<int> ints2)\n"
"{\n" "{\n"
" std::vector<int> ints1;\n"
" std::vector<int> ints2;\n"
" std::vector<int>::iterator it = std::inplace_merge(ints1.begin(), std::advance(ints2.rbegin(), 5), ints1.end());\n" " std::vector<int>::iterator it = std::inplace_merge(ints1.begin(), std::advance(ints2.rbegin(), 5), ints1.end());\n"
"}"); "}");
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
} }
void iterator8() { void iterator8() {
check("void foo()\n" check("void foo(std::vector<int> ints1, std::vector<int> ints2)\n"
"{\n" "{\n"
" std::vector<int> ints1;\n"
" std::vector<int> ints2;\n"
" std::vector<int>::iterator it = std::find_first_of(ints1.begin(), ints2.end(), ints1.begin(), ints1.end());\n" " std::vector<int>::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()); errout.str());
check("void foo()\n" check("void foo(std::vector<int> ints1, std::vector<int> ints2)\n"
"{\n" "{\n"
" std::vector<int> ints1;\n"
" std::vector<int> ints2;\n"
" std::vector<int>::iterator it = std::find_first_of(ints1.begin(), ints1.end(), ints2.begin(), ints1.end());\n" " std::vector<int>::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()); errout.str());
check("void foo()\n" check("void foo(std::vector<int> ints1, std::vector<int> ints2)\n"
"{\n" "{\n"
" std::vector<int> ints1;\n"
" std::vector<int> ints2;\n"
" std::vector<int>::iterator it = std::find_first_of(foo.bar.begin(), foo.bar.end()-6, ints2.begin(), ints1.end());\n" " std::vector<int>::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()); errout.str());
check("void foo()\n" check("void foo(std::vector<int> ints1, std::vector<int> ints2)\n"
"{\n" "{\n"
" std::vector<int> ints1;\n"
" std::vector<int> ints2;\n"
" std::vector<int>::iterator it = std::find_first_of(ints1.begin(), ints1.end(), ints2.begin(), ints2.end());\n" " std::vector<int>::iterator it = std::find_first_of(ints1.begin(), ints1.end(), ints2.begin(), ints2.end());\n"
"}"); "}");
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
@ -3397,8 +3383,7 @@ private:
"}"); "}");
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
check("void f() {\n" check("void f(std::vector<int> a) {\n"
" std::vector<int> a;\n"
" std::remove(a.begin(), a.end(), val);\n" " std::remove(a.begin(), a.end(), val);\n"
" std::remove_if(a.begin(), a.end(), val);\n" " std::remove_if(a.begin(), a.end(), val);\n"
" std::unique(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" " a.erase(std::remove(a.begin(), a.end(), val));\n"
" std::remove(\"foo.txt\");\n" " std::remove(\"foo.txt\");\n"
"}"); "}");
ASSERT_EQUALS("[test.cpp:3]: (warning) Return value of std::remove() ignored. Elements remain in container.\n" ASSERT_EQUALS("[test.cpp:2]: (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:3]: (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()); "[test.cpp:4]: (warning) Return value of std::unique() ignored. Elements remain in container.\n", errout.str());
// #4431 - fp // #4431 - fp
check("bool f() {\n" check("bool f() {\n"
@ -4648,7 +4633,7 @@ private:
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
} }
void checkKnownEmptyContainerLoop() { void checkKnownEmptyContainer() {
check("void f() {\n" check("void f() {\n"
" std::vector<int> v;\n" " std::vector<int> v;\n"
" for(auto x:v) {}\n" " for(auto x:v) {}\n"
@ -4676,6 +4661,20 @@ private:
"}\n", "}\n",
true); true);
ASSERT_EQUALS("", errout.str()); ASSERT_EQUALS("", errout.str());
check("void f() {\n"
" std::vector<int> 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<int> v;\n"
" v.insert(v.end(), 1);\n"
"}\n",
true);
ASSERT_EQUALS("", errout.str());
} }
void checkMutexes() { void checkMutexes() {