Skip to content

Fix 15116: False positive: stlFindInsert (modifying existing element) #8975

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
pfultz2 wants to merge 5 commits into
base: main
Choose a base branch
from
+166 −24
Open

Fix 15116: False positive: stlFindInsert (modifying existing element) #8975

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2e830f2
Fix 15116: False positive: stlFindInsert (modifying existing element)
pfultz2 Oct 28, 2026
9b6e9f8
Improve message
pfultz2 Oct 29, 2026
26e0795
Format
pfultz2 Oct 29, 2026
10bd8fa
Fix ci
pfultz2 Oct 30, 2026
f6cb7b6
Merge branch 'main' into stl-find-insert-msg-use-it
pfultz2 Oct 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ $(libcppdir)/checks.o: lib/checks.cpp lib/check.h lib/check64bit.h lib/checkasse
$(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
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checksizeof.cpp

$(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
$(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
$(CXX) ${INCLUDE_FOR_LIB} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkstl.cpp

$(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
Expand Down
86 changes: 65 additions & 21 deletions lib/checkstl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@
#include "checkstl.h"

#include "astutils.h"
#include "calculate.h"
#include "errortypes.h"
#include "library.h"
#include "mathlib.h"
#include "refthunk.h"
#include "pathanalysis.h"
#include "refthunk.h"
#include "settings.h"
#include "standards.h"
#include "symboldatabase.h"
Expand Down Expand Up @@ -1734,34 +1735,77 @@ void CheckStlImpl::checkFindInsert()
if (!valueTok2)
continue;
if (isSameExpression(true, valueTok, valueTok2, mSettings, true, true)) {
checkFindInsertError(valueTok);
// Both branches set the same value; the replacement depends on both branches, so no suggestion
checkFindInsertError(valueTok, nullptr);
}
} else {
checkFindInsertError(valueTok);
checkFindInsertError(valueTok, condTok);
}
}
}
}

void CheckStlImpl::checkFindInsertError(const Token *tok)
{
std::string replaceExpr;
if (tok && Token::simpleMatch(tok->astParent(), "=") && tok == tok->astParent()->astOperand2() && Token::simpleMatch(tok->astParent()->astOperand1(), "[")) {
if (mSettings.standards.cpp < Standards::CPP11)
// We will recommend using emplace/try_emplace instead
return;
const std::string f = (mSettings.standards.cpp < Standards::CPP17) ? "emplace" : "try_emplace";
replaceExpr = " Instead of '" + tok->astParent()->expressionString() + "' consider using '" +
tok->astParent()->astOperand1()->astOperand1()->expressionString() +
"." + f + "(" +
tok->astParent()->astOperand1()->astOperand2()->expressionString() +
", " +
tok->expressionString() +
");'.";
/** When is the condition of the 'if' statement checked by checkFindInsert() true? */
enum class FindInsertCond : std::uint8_t { UNKNOWN, FOUND, NOT_FOUND };

static FindInsertCond getFindInsertCond(const Token* condTok)
{
const Token* rhs = condTok->astOperand2();
if (!rhs)
return FindInsertCond::UNKNOWN;
if (Token::simpleMatch(condTok->astOperand1()->astOperand1(), ". find (")) {
// c.find(k) == c.end() / c.find(k) != c.end()
if (!Token::simpleMatch(rhs, "(") || !Token::Match(rhs->astOperand1(), ". end|cend ("))
return FindInsertCond::UNKNOWN;
if (condTok->str() == "==")
return FindInsertCond::NOT_FOUND;
if (condTok->str() == "!=")
return FindInsertCond::FOUND;
return FindInsertCond::UNKNOWN;
}
// c.count(k) == 0, c.count(k) > 0, ...: count() yields 0 or 1
if (!rhs->hasKnownIntValue())
return FindInsertCond::UNKNOWN;
const MathLib::bigint n = rhs->getKnownIntValue();
const bool found = calculate<MathLib::bigint, MathLib::bigint>(condTok->str(), 1, n) != 0;
const bool notFound = calculate<MathLib::bigint, MathLib::bigint>(condTok->str(), 0, n) != 0;
if (found == notFound)
return FindInsertCond::UNKNOWN;
return found ? FindInsertCond::FOUND : FindInsertCond::NOT_FOUND;
}

void CheckStlImpl::checkFindInsertError(const Token* tok, const Token* condTok)
{
std::string suggestion;
std::string method = "insertion";
if (tok && condTok && Token::simpleMatch(tok->astParent(), "=") && tok == tok->astParent()->astOperand2() &&
Token::simpleMatch(tok->astParent()->astOperand1(), "[")) {
const std::string containerExpr = tok->astParent()->astOperand1()->astOperand1()->expressionString();
const std::string keyExpr = tok->astParent()->astOperand1()->astOperand2()->expressionString();
switch (getFindInsertCond(condTok)) {
case FindInsertCond::NOT_FOUND: {
if (mSettings.standards.cpp < Standards::CPP11)
// We will recommend using emplace/try_emplace instead
return;
const std::string f = (mSettings.standards.cpp < Standards::CPP17) ? "emplace" : "try_emplace";
suggestion = " Instead of '" + tok->astParent()->expressionString() + "' consider using '" + containerExpr +
"." + f + "(" + keyExpr + ", " + tok->expressionString() + ");'.";
break;
}
case FindInsertCond::FOUND:
// The key is present, so the element is modified rather than inserted; try_emplace() would not do that
method = "modification";
suggestion = " Instead of '" + tok->astParent()->expressionString() +
"' consider assigning the value through the iterator returned by '" + containerExpr +
".find(" + keyExpr + ")'.";
break;
case FindInsertCond::UNKNOWN:
break;
}
}
std::string msg = "Searching before " + method + " is not necessary." + suggestion;

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

/**
Expand Down Expand Up @@ -3899,7 +3943,7 @@ void CheckStl::getErrorMessages(ErrorLogger& errorLogger, const Settings& settin
c.stlBoundariesError(nullptr);
c.if_findError(nullptr, false);
c.if_findError(nullptr, true);
c.checkFindInsertError(nullptr);
c.checkFindInsertError(nullptr, nullptr);
c.string_c_strError(nullptr);
c.string_c_strReturn(nullptr);
c.string_c_strParam(nullptr, 0);
Expand Down
3 changes: 2 additions & 1 deletion lib/checkstl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,8 @@ class CPPCHECKLIB CheckStlImpl : public CheckImpl {
void sameIteratorExpressionError(const Token *tok);
void stlBoundariesError(const Token* tok);
void if_findError(const Token* tok, bool str);
void checkFindInsertError(const Token *tok);
/** @param condTok the comparison in the if condition, nullptr => no specific suggestion */
void checkFindInsertError(const Token* tok, const Token* condTok);
void sizeError(const Token* tok);
void redundantIfRemoveError(const Token* tok);
void invalidContainerLoopError(const Token* tok, const Token* loopTok, ErrorPath errorPath);
Expand Down
2 changes: 1 addition & 1 deletion oss-fuzz/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ $(libcppdir)/checks.o: ../lib/checks.cpp ../lib/check.h ../lib/check64bit.h ../l
$(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
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checksizeof.cpp

$(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
$(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
$(CXX) ${LIB_FUZZING_ENGINE} $(CPPFLAGS) $(CXXFLAGS) -c -o $@ $(libcppdir)/checkstl.cpp

$(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
Expand Down
97 changes: 97 additions & 0 deletions test/teststl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7480,6 +7480,103 @@ class TestStl : public TestFixture {
"}\n"); // TODO: use s?
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());
}

// The key is found => the value is assigned, try_emplace() would not do that
check("void f() {\n"
" std::map<int, bool> m{{1, false}};\n"
" if (m.count(1) != 0)\n"
" m[1] = true;\n"
"}\n");
ASSERT_EQUALS(
"[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",
errout_str());

check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
" if (m.find(x) != m.end())\n"
" m[x] = 1;\n"
"}\n");
ASSERT_EQUALS(
"[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",
errout_str());

check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
" if (m.count(x) > 0)\n"
" m[x] = 1;\n"
"}\n");
ASSERT_EQUALS(
"[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",
errout_str());

check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
" if (m.count(x) == 1)\n"
" m[x] = 1;\n"
"}\n");
ASSERT_EQUALS(
"[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",
errout_str());

// The key is not found => try_emplace()
check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
" if (m.count(x) == 0)\n"
" m[x] = 1;\n"
"}\n");
ASSERT_EQUALS(
"[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",
errout_str());

check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
" if (m.count(x) < 1)\n"
" m[x] = 1;\n"
"}\n");
ASSERT_EQUALS(
"[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",
errout_str());

// Unknown condition => no suggestion
check("void f(std::map<unsigned, unsigned>& m, unsigned x, unsigned y) {\n"
" if (m.count(x) == y)\n"
" m[x] = 1;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3:16]: (performance) Searching before insertion is not necessary. [stlFindInsert]\n",
errout_str());

// Both branches set the value => no suggestion
check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
" if (m.find(x) == m.end())\n"
" m[x] = 1;\n"
" else\n"
" m[x] = 1;\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3:16]: (performance) Searching before insertion is not necessary. [stlFindInsert]\n",
errout_str());

check("void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
" if (m.find(x) != m.end())\n"
" m[x] = 1;\n"
" else\n"
" m.emplace(x, 1);\n"
"}\n");
ASSERT_EQUALS("[test.cpp:3:16]: (performance) Searching before insertion is not necessary. [stlFindInsert]\n",
errout_str());

// The suggestion for a found key does not depend on the C++ standard
{
Settings s = settings;
const char code[] = "void f(std::map<unsigned, unsigned>& m, unsigned x) {\n"
" if (m.count(x) != 0)\n"
" m[x] = 1;\n"
"}\n";
s.standards.cpp = Standards::CPP03;
check(code, s);
ASSERT_EQUALS(
"[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",
errout_str());
s.standards.cpp = Standards::CPP11;
check(code, s);
ASSERT_EQUALS(
"[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",
errout_str());
}
}

void checkKnownEmptyContainer() {
Expand Down
Loading