diff --git a/Makefile b/Makefile index daed0997479..b4b35a08200 100644 --- a/Makefile +++ b/Makefile @@ -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 diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index a4e8a92c188..f07e8aa7166 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -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" @@ -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(condTok->str(), 1, n) != 0; + const bool notFound = calculate(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); } /** @@ -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); diff --git a/lib/checkstl.h b/lib/checkstl.h index 0f9a70a7a15..059ddb7466e 100644 --- a/lib/checkstl.h +++ b/lib/checkstl.h @@ -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); diff --git a/oss-fuzz/Makefile b/oss-fuzz/Makefile index 5b8a1d159d4..081aa035cdc 100644 --- a/oss-fuzz/Makefile +++ b/oss-fuzz/Makefile @@ -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 diff --git a/test/teststl.cpp b/test/teststl.cpp index bb20d2d8182..1e0dd48d698 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -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 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& 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& 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& 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& 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& 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& 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& 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& 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& 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() {