diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 1b3a8edb8b8..028a29f7ac0 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -2899,10 +2899,21 @@ void CheckStl::useStlAlgorithm() return !astIsContainer(tok); // don't warn for containers, where overloaded operators can be costly }; - auto isConditionWithoutSideEffects = [this](const Token* tok) -> bool { + enum class ConditionOpType : std::uint8_t { OTHER, MIN, MAX }; + auto isConditionWithoutSideEffects = [this](const Token* tok, ConditionOpType& type) -> bool { if (!Token::simpleMatch(tok, "{") || !Token::simpleMatch(tok->previous(), ")")) return false; - return isConstExpression(tok->linkAt(-1)->astOperand2(), mSettings->library); + const Token* condTok = tok->linkAt(-1)->astOperand2(); + if (isConstExpression(condTok, mSettings->library)) { + if (condTok->str() == "<") + type = ConditionOpType::MIN; + else if (condTok->str() == ">") + type = ConditionOpType::MAX; + else + type = ConditionOpType::OTHER; + return true; + } + return false; }; auto isAccumulation = [](const Token* tok, int varId) { @@ -3035,14 +3046,27 @@ void CheckStl::useStlAlgorithm() else algo = "std::replace_if"; } else { + ConditionOpType type{}; if (addByOne(assignTok, assignVarId)) algo = "std::count_if"; else if (accumulateBoolLiteral(assignTok, assignVarId)) algo = "std::any_of, std::all_of, std::none_of, or std::accumulate"; else if (assignTok->str() != "=") algo = "std::accumulate"; - else if (hasBreak && isConditionWithoutSideEffects(condBodyTok)) - algo = "std::any_of, std::all_of, std::none_of"; + else if (isConditionWithoutSideEffects(condBodyTok, type)) { + if (hasBreak) + algo = "std::any_of, std::all_of, std::none_of"; + else if (assignTok->astOperand2()->varId() == loopVar->varId()) { + if (type == ConditionOpType::MIN) + algo = "std::min_element"; + else if (type == ConditionOpType::MAX) + algo = "std::max_element"; + else + continue; + } + else + continue; + } else continue; } diff --git a/test/teststl.cpp b/test/teststl.cpp index da6b1c1fd5e..ee1224d4656 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -5859,7 +5859,7 @@ class TestStl : public TestFixture { dinit(CheckOptions, $.inconclusive = true)); ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::accumulate algorithm instead of a raw loop.\n", errout_str()); - check("void f(const std::vector& v) {\n" + check("void f(const std::vector& v) {\n" // #9091 " int maxY = 0;\n" " for (int y : v) {\n" " if (y > maxY)\n" @@ -5867,9 +5867,18 @@ class TestStl : public TestFixture { " }\n" "}\n", dinit(CheckOptions, $.inconclusive = true)); - TODO_ASSERT_EQUALS("[test.cpp:5]: (style) Consider using std::max_element algorithm instead of a raw loop.\n", - "", - errout_str()); + ASSERT_EQUALS("[test.cpp:5]: (style) Consider using std::max_element algorithm instead of a raw loop.\n", errout_str()); + + check("int f(const std::vector& v) {\n" + " int minY = 0;\n" + " for (int y : v) {\n" + " if (y < minY)\n" + " minY = y;\n" + " }\n" + " return minY;\n" + "}\n", + dinit(CheckOptions, $.inconclusive = true)); + ASSERT_EQUALS("[test.cpp:5]: (style) Consider using std::min_element algorithm instead of a raw loop.\n", errout_str()); } void loopAlgoMultipleReturn()