Skip to content

Commit 0c4dc89

Browse files
pfultz2pfultz2
authored
Fix 15116: False positive: stlFindInsert (modifying existing element) (#8975)
1 parent ccd4104 commit 0c4dc89

5 files changed

Lines changed: 166 additions & 24 deletions

File tree

‎Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -568,7 +568,7 @@ $(libcppdir)/checks.o: lib/checks.cpp lib/check.h lib/check64bit.h lib/checkasse
568568
$(libcppdir)/checksizeof.o: lib/checksizeof.cpp lib/check.h lib/checkers.h lib/checkimpl.h lib/checksizeof.h lib/config.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/refthunk.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h
569569
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checksizeof.cpp
570570

571-
$(libcppdir)/checkstl.o: lib/checkstl.cpp lib/astutils.h lib/check.h lib/checkers.h lib/checkimpl.h lib/checknullpointer.h lib/checkstl.h lib/config.h lib/errortypes.h lib/library.h lib/mathlib.h lib/pathanalysis.h lib/platform.h lib/refthunk.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h
571+
$(libcppdir)/checkstl.o: lib/checkstl.cpp lib/astutils.h lib/calculate.h lib/check.h lib/checkers.h lib/checkimpl.h lib/checknullpointer.h lib/checkstl.h lib/config.h lib/errortypes.h lib/library.h lib/mathlib.h lib/pathanalysis.h lib/platform.h lib/refthunk.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/valueflow.h lib/vfvalue.h
572572
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkstl.cpp
573573

574574
$(libcppdir)/checkstring.o: lib/checkstring.cpp lib/astutils.h lib/check.h lib/checkers.h lib/checkimpl.h lib/checkstring.h lib/config.h lib/errortypes.h lib/library.h lib/mathlib.h lib/platform.h lib/refthunk.h lib/settings.h lib/smallvector.h lib/sourcelocation.h lib/standards.h lib/symboldatabase.h lib/templatesimplifier.h lib/token.h lib/tokenize.h lib/tokenlist.h lib/utils.h lib/vfvalue.h

‎lib/checkstl.cpp‎

Lines changed: 65 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,12 @@
1919
#include "checkstl.h"
2020

2121
#include "astutils.h"
22+
#include "calculate.h"
2223
#include "errortypes.h"
2324
#include "library.h"
2425
#include "mathlib.h"
25-
#include "refthunk.h"
2626
#include "pathanalysis.h"
27+
#include "refthunk.h"
2728
#include "settings.h"
2829
#include "standards.h"
2930
#include "symboldatabase.h"
@@ -1734,34 +1735,77 @@ void CheckStlImpl::checkFindInsert()
17341735
if (!valueTok2)
17351736
continue;
17361737
if (isSameExpression(true, valueTok, valueTok2, mSettings, true, true)) {
1737-
checkFindInsertError(valueTok);
1738+
// Both branches set the same value; the replacement depends on both branches, so no suggestion
1739+
checkFindInsertError(valueTok, nullptr);
17381740
}
17391741
} else {
1740-
checkFindInsertError(valueTok);
1742+
checkFindInsertError(valueTok, condTok);
17411743
}
17421744
}
17431745
}
17441746
}
17451747

1746-
void CheckStlImpl::checkFindInsertError(const Token *tok)
1747-
{
1748-
std::string replaceExpr;
1749-
if (tok && Token::simpleMatch(tok->astParent(), "=") && tok == tok->astParent()->astOperand2() && Token::simpleMatch(tok->astParent()->astOperand1(), "[")) {
1750-
if (mSettings.standards.cpp < Standards::CPP11)
1751-
// We will recommend using emplace/try_emplace instead
1752-
return;
1753-
const std::string f = (mSettings.standards.cpp < Standards::CPP17) ? "emplace" : "try_emplace";
1754-
replaceExpr = " Instead of '" + tok->astParent()->expressionString() + "' consider using '" +
1755-
tok->astParent()->astOperand1()->astOperand1()->expressionString() +
1756-
"." + f + "(" +
1757-
tok->astParent()->astOperand1()->astOperand2()->expressionString() +
1758-
", " +
1759-
tok->expressionString() +
1760-
");'.";
1748+
/** When is the condition of the 'if' statement checked by checkFindInsert() true? */
1749+
enum class FindInsertCond : std::uint8_t { UNKNOWN, FOUND, NOT_FOUND };
1750+
1751+
static FindInsertCond getFindInsertCond(const Token* condTok)
1752+
{
1753+
const Token* rhs = condTok->astOperand2();
1754+
if (!rhs)
1755+
return FindInsertCond::UNKNOWN;
1756+
if (Token::simpleMatch(condTok->astOperand1()->astOperand1(), ". find (")) {
1757+
// c.find(k) == c.end() / c.find(k) != c.end()
1758+
if (!Token::simpleMatch(rhs, "(") || !Token::Match(rhs->astOperand1(), ". end|cend ("))
1759+
return FindInsertCond::UNKNOWN;
1760+
if (condTok->str() == "==")
1761+
return FindInsertCond::NOT_FOUND;
1762+
if (condTok->str() == "!=")
1763+
return FindInsertCond::FOUND;
1764+
return FindInsertCond::UNKNOWN;
1765+
}
1766+
// c.count(k) == 0, c.count(k) > 0, ...: count() yields 0 or 1
1767+
if (!rhs->hasKnownIntValue())
1768+
return FindInsertCond::UNKNOWN;
1769+
const MathLib::bigint n = rhs->getKnownIntValue();
1770+
const bool found = calculate<MathLib::bigint, MathLib::bigint>(condTok->str(), 1, n) != 0;
1771+
const bool notFound = calculate<MathLib::bigint, MathLib::bigint>(condTok->str(), 0, n) != 0;
1772+
if (found == notFound)
1773+
return FindInsertCond::UNKNOWN;
1774+
return found ? FindInsertCond::FOUND : FindInsertCond::NOT_FOUND;
1775+
}
1776+
1777+
void CheckStlImpl::checkFindInsertError(const Token* tok, const Token* condTok)
1778+
{
1779+
std::string suggestion;
1780+
std::string method = "insertion";
1781+
if (tok && condTok && Token::simpleMatch(tok->astParent(), "=") && tok == tok->astParent()->astOperand2() &&
1782+
Token::simpleMatch(tok->astParent()->astOperand1(), "[")) {
1783+
const std::string containerExpr = tok->astParent()->astOperand1()->astOperand1()->expressionString();
1784+
const std::string keyExpr = tok->astParent()->astOperand1()->astOperand2()->expressionString();
1785+
switch (getFindInsertCond(condTok)) {
1786+
case FindInsertCond::NOT_FOUND: {
1787+
if (mSettings.standards.cpp < Standards::CPP11)
1788+
// We will recommend using emplace/try_emplace instead
1789+
return;
1790+
const std::string f = (mSettings.standards.cpp < Standards::CPP17) ? "emplace" : "try_emplace";
1791+
suggestion = " Instead of '" + tok->astParent()->expressionString() + "' consider using '" + containerExpr +
1792+
"." + f + "(" + keyExpr + ", " + tok->expressionString() + ");'.";
1793+
break;
1794+
}
1795+
case FindInsertCond::FOUND:
1796+
// The key is present, so the element is modified rather than inserted; try_emplace() would not do that
1797+
method = "modification";
1798+
suggestion = " Instead of '" + tok->astParent()->expressionString() +
1799+
"' consider assigning the value through the iterator returned by '" + containerExpr +
1800+
".find(" + keyExpr + ")'.";
1801+
break;
1802+
case FindInsertCond::UNKNOWN:
1803+
break;
1804+
}
17611805
}
1806+
std::string msg = "Searching before " + method + " is not necessary." + suggestion;
17621807

1763-
reportError(
1764-
tok, Severity::performance, "stlFindInsert", "Searching before insertion is not necessary." + replaceExpr, CWE398, Certainty::normal);
1808+
reportError(tok, Severity::performance, "stlFindInsert", msg, CWE398, Certainty::normal);
17651809
}
17661810

17671811
/**
@@ -3899,7 +3943,7 @@ void CheckStl::getErrorMessages(ErrorLogger& errorLogger, const Settings& settin
38993943
c.stlBoundariesError(nullptr);
39003944
c.if_findError(nullptr, false);
39013945
c.if_findError(nullptr, true);
3902-
c.checkFindInsertError(nullptr);
3946+
c.checkFindInsertError(nullptr, nullptr);
39033947
c.string_c_strError(nullptr);
39043948
c.string_c_strReturn(nullptr);
39053949
c.string_c_strParam(nullptr, 0);

‎lib/checkstl.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,8 @@ class CPPCHECKLIB CheckStlImpl : public CheckImpl {
219219
void sameIteratorExpressionError(const Token *tok);
220220
void stlBoundariesError(const Token* tok);
221221
void if_findError(const Token* tok, bool str);
222-
void checkFindInsertError(const Token *tok);
222+
/** @param condTok the comparison in the if condition, nullptr => no specific suggestion */
223+
void checkFindInsertError(const Token* tok, const Token* condTok);
223224
void sizeError(const Token* tok);
224225
void redundantIfRemoveError(const Token* tok);
225226
void invalidContainerLoopError(const Token* tok, const Token* loopTok, ErrorPath errorPath);

‎oss-fuzz/Makefile‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ $(libcppdir)/checks.o: ../lib/checks.cpp ../lib/check.h ../lib/check64bit.h ../l
237237
$(libcppdir)/checksizeof.o: ../lib/checksizeof.cpp ../lib/check.h ../lib/checkers.h ../lib/checkimpl.h ../lib/checksizeof.h ../lib/config.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/platform.h ../lib/refthunk.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/vfvalue.h
238238
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checksizeof.cpp
239239

240-
$(libcppdir)/checkstl.o: ../lib/checkstl.cpp ../lib/astutils.h ../lib/check.h ../lib/checkers.h ../lib/checkimpl.h ../lib/checknullpointer.h ../lib/checkstl.h ../lib/config.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/pathanalysis.h ../lib/platform.h ../lib/refthunk.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/valueflow.h ../lib/vfvalue.h
240+
$(libcppdir)/checkstl.o: ../lib/checkstl.cpp ../lib/astutils.h ../lib/calculate.h ../lib/check.h ../lib/checkers.h ../lib/checkimpl.h ../lib/checknullpointer.h ../lib/checkstl.h ../lib/config.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/pathanalysis.h ../lib/platform.h ../lib/refthunk.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/valueflow.h ../lib/vfvalue.h
241241
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkstl.cpp
242242

243243
$(libcppdir)/checkstring.o: ../lib/checkstring.cpp ../lib/astutils.h ../lib/check.h ../lib/checkers.h ../lib/checkimpl.h ../lib/checkstring.h ../lib/config.h ../lib/errortypes.h ../lib/library.h ../lib/mathlib.h ../lib/platform.h ../lib/refthunk.h ../lib/settings.h ../lib/smallvector.h ../lib/sourcelocation.h ../lib/standards.h ../lib/symboldatabase.h ../lib/templatesimplifier.h ../lib/token.h ../lib/tokenize.h ../lib/tokenlist.h ../lib/utils.h ../lib/vfvalue.h

‎test/teststl.cpp‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7480,6 +7480,103 @@ class TestStl : public TestFixture {
74807480
"}\n"); // TODO: use s?
74817481
ASSERT_EQUALS("[test.cpp:7:17]: (performance) Searching before insertion is not necessary. Instead of 'x[5]=data' consider using 'x.try_emplace(5, data);'. [stlFindInsert]\n", errout_str());
74827482
}
7483+
7484+
// The key is found => the value is assigned, try_emplace() would not do that
7485+
check("void f() {\n"
7486+
" std::map<int, bool> m{{1, false}};\n"
7487+
" if (m.count(1) != 0)\n"
7488+
" m[1] = true;\n"
7489+
"}\n");
7490+
ASSERT_EQUALS(
7491+
"[test.cpp:4:16]: (performance) Searching before modification is not necessary. Instead of 'm[1]=true' consider assigning the value through the iterator returned by 'm.find(1)'. [stlFindInsert]\n",
7492+
errout_str());
7493+
7494+
check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
7495+
" if (m.find(x) != m.end())\n"
7496+
" m[x] = 1;\n"
7497+
"}\n");
7498+
ASSERT_EQUALS(
7499+
"[test.cpp:3:16]: (performance) Searching before modification is not necessary. Instead of 'm[x]=1' consider assigning the value through the iterator returned by 'm.find(x)'. [stlFindInsert]\n",
7500+
errout_str());
7501+
7502+
check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
7503+
" if (m.count(x) > 0)\n"
7504+
" m[x] = 1;\n"
7505+
"}\n");
7506+
ASSERT_EQUALS(
7507+
"[test.cpp:3:16]: (performance) Searching before modification is not necessary. Instead of 'm[x]=1' consider assigning the value through the iterator returned by 'm.find(x)'. [stlFindInsert]\n",
7508+
errout_str());
7509+
7510+
check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
7511+
" if (m.count(x) == 1)\n"
7512+
" m[x] = 1;\n"
7513+
"}\n");
7514+
ASSERT_EQUALS(
7515+
"[test.cpp:3:16]: (performance) Searching before modification is not necessary. Instead of 'm[x]=1' consider assigning the value through the iterator returned by 'm.find(x)'. [stlFindInsert]\n",
7516+
errout_str());
7517+
7518+
// The key is not found => try_emplace()
7519+
check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
7520+
" if (m.count(x) == 0)\n"
7521+
" m[x] = 1;\n"
7522+
"}\n");
7523+
ASSERT_EQUALS(
7524+
"[test.cpp:3:16]: (performance) Searching before insertion is not necessary. Instead of 'm[x]=1' consider using 'm.try_emplace(x, 1);'. [stlFindInsert]\n",
7525+
errout_str());
7526+
7527+
check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
7528+
" if (m.count(x) < 1)\n"
7529+
" m[x] = 1;\n"
7530+
"}\n");
7531+
ASSERT_EQUALS(
7532+
"[test.cpp:3:16]: (performance) Searching before insertion is not necessary. Instead of 'm[x]=1' consider using 'm.try_emplace(x, 1);'. [stlFindInsert]\n",
7533+
errout_str());
7534+
7535+
// Unknown condition => no suggestion
7536+
check("void f(std::map<unsigned, unsigned>& m, unsigned x, unsigned y) {\n"
7537+
" if (m.count(x) == y)\n"
7538+
" m[x] = 1;\n"
7539+
"}\n");
7540+
ASSERT_EQUALS("[test.cpp:3:16]: (performance) Searching before insertion is not necessary. [stlFindInsert]\n",
7541+
errout_str());
7542+
7543+
// Both branches set the value => no suggestion
7544+
check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
7545+
" if (m.find(x) == m.end())\n"
7546+
" m[x] = 1;\n"
7547+
" else\n"
7548+
" m[x] = 1;\n"
7549+
"}\n");
7550+
ASSERT_EQUALS("[test.cpp:3:16]: (performance) Searching before insertion is not necessary. [stlFindInsert]\n",
7551+
errout_str());
7552+
7553+
check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
7554+
" if (m.find(x) != m.end())\n"
7555+
" m[x] = 1;\n"
7556+
" else\n"
7557+
" m.emplace(x, 1);\n"
7558+
"}\n");
7559+
ASSERT_EQUALS("[test.cpp:3:16]: (performance) Searching before insertion is not necessary. [stlFindInsert]\n",
7560+
errout_str());
7561+
7562+
// The suggestion for a found key does not depend on the C++ standard
7563+
{
7564+
Settings s = settings;
7565+
const char code[] = "void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
7566+
" if (m.count(x) != 0)\n"
7567+
" m[x] = 1;\n"
7568+
"}\n";
7569+
s.standards.cpp = Standards::CPP03;
7570+
check(code, s);
7571+
ASSERT_EQUALS(
7572+
"[test.cpp:3:16]: (performance) Searching before modification is not necessary. Instead of 'm[x]=1' consider assigning the value through the iterator returned by 'm.find(x)'. [stlFindInsert]\n",
7573+
errout_str());
7574+
s.standards.cpp = Standards::CPP11;
7575+
check(code, s);
7576+
ASSERT_EQUALS(
7577+
"[test.cpp:3:16]: (performance) Searching before modification is not necessary. Instead of 'm[x]=1' consider assigning the value through the iterator returned by 'm.find(x)'. [stlFindInsert]\n",
7578+
errout_str());
7579+
}
74837580
}
74847581

74857582
void checkKnownEmptyContainer() {

0 commit comments

Comments
 (0)