From b1abaf8809dba3221c8d8a0f025fcda667f90dac Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Sun, 18 Dec 2022 16:52:04 +0100 Subject: [PATCH] Fix FP useStlAlgorithm (don't suggest std::accumulate when nothing is accumulated) (#4647) --- lib/astutils.cpp | 4 ++-- lib/astutils.h | 2 +- lib/checkbool.cpp | 2 +- lib/checkfunctions.cpp | 4 ++-- lib/checkother.cpp | 2 +- lib/checkstl.cpp | 12 +++++++++++- lib/reverseanalyzer.cpp | 2 +- lib/valueflow.cpp | 8 ++++---- test/teststl.cpp | 19 +++++++++++++++++++ 9 files changed, 42 insertions(+), 13 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 5e694ac5b..0ff701174 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1922,7 +1922,7 @@ bool isConstFunctionCall(const Token* ftok, const Library& library) return true; } -bool isConstExpression(const Token *tok, const Library& library, bool pure, bool cpp) +bool isConstExpression(const Token *tok, const Library& library, bool cpp) { if (!tok) return true; @@ -1941,7 +1941,7 @@ bool isConstExpression(const Token *tok, const Library& library, bool pure, bool // bailout when we see ({..}) if (tok->str() == "{") return false; - return isConstExpression(tok->astOperand1(), library, pure, cpp) && isConstExpression(tok->astOperand2(), library, pure, cpp); + return isConstExpression(tok->astOperand1(), library, cpp) && isConstExpression(tok->astOperand2(), library, cpp); } bool isWithoutSideEffects(bool cpp, const Token* tok, bool checkArrayAccess, bool checkReference) diff --git a/lib/astutils.h b/lib/astutils.h index d60ec3a5c..082f8addf 100644 --- a/lib/astutils.h +++ b/lib/astutils.h @@ -266,7 +266,7 @@ bool isOppositeExpression(bool cpp, const Token * const tok1, const Token * cons bool isConstFunctionCall(const Token* ftok, const Library& library); -bool isConstExpression(const Token *tok, const Library& library, bool pure, bool cpp); +bool isConstExpression(const Token *tok, const Library& library, bool cpp); bool isWithoutSideEffects(bool cpp, const Token* tok, bool checkArrayAccess = false, bool checkReference = true); diff --git a/lib/checkbool.cpp b/lib/checkbool.cpp index 0755ad41d..9823a0f7e 100644 --- a/lib/checkbool.cpp +++ b/lib/checkbool.cpp @@ -116,7 +116,7 @@ void CheckBool::checkBitwiseOnBoolean() if (tok->str() == "|" && !isConvertedToBool(tok) && !(isBoolOp1 && isBoolOp2)) continue; // first operand will always be evaluated - if (!isConstExpression(tok->astOperand2(), mSettings->library, true, mTokenizer->isCPP())) + if (!isConstExpression(tok->astOperand2(), mSettings->library, mTokenizer->isCPP())) continue; if (tok->astOperand2()->variable() && tok->astOperand2()->variable()->nameToken() == tok->astOperand2()) continue; diff --git a/lib/checkfunctions.cpp b/lib/checkfunctions.cpp index f3553b20f..f14bb3eaf 100644 --- a/lib/checkfunctions.cpp +++ b/lib/checkfunctions.cpp @@ -725,10 +725,10 @@ void CheckFunctions::useStandardLibrary() const auto& secondOp = condToken->str(); const bool isLess = "<" == secondOp && - isConstExpression(condToken->astOperand2(), mSettings->library, true, mTokenizer->isCPP()) && + isConstExpression(condToken->astOperand2(), mSettings->library, mTokenizer->isCPP()) && condToken->astOperand1()->varId() == idxVarId; const bool isMore = ">" == secondOp && - isConstExpression(condToken->astOperand1(), mSettings->library, true, mTokenizer->isCPP()) && + isConstExpression(condToken->astOperand1(), mSettings->library, mTokenizer->isCPP()) && condToken->astOperand2()->varId() == idxVarId; if (!(isLess || isMore)) diff --git a/lib/checkother.cpp b/lib/checkother.cpp index f69f96260..0400cf671 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2521,7 +2521,7 @@ void CheckOther::checkDuplicateExpression() &errorPath) && isWithoutSideEffects(mTokenizer->isCPP(), tok->astOperand2())) duplicateExpressionError(tok->astOperand2(), tok->astOperand1()->astOperand2(), tok, errorPath); - else if (tok->astOperand2() && isConstExpression(tok->astOperand1(), mSettings->library, true, mTokenizer->isCPP())) { + else if (tok->astOperand2() && isConstExpression(tok->astOperand1(), mSettings->library, mTokenizer->isCPP())) { auto checkDuplicate = [&](const Token* exp1, const Token* exp2, const Token* ast1) { if (isSameExpression(mTokenizer->isCPP(), true, exp1, exp2, mSettings->library, true, true, &errorPath) && isWithoutSideEffects(mTokenizer->isCPP(), exp1) && diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 4b008348c..b59576680 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -2708,6 +2708,12 @@ void CheckStl::useStlAlgorithm() return !astIsContainer(tok); // don't warn for containers, where overloaded operators can be costly }; + auto isConditionWithoutSideEffects = [this](const Token* tok) -> bool { + if (!Token::simpleMatch(tok, "{") || !Token::simpleMatch(tok->previous(), ")")) + return false; + return isConstExpression(tok->previous()->link()->astOperand2(), mSettings->library, true); + }; + for (const Scope *function : mTokenizer->getSymbolDatabase()->functionScopes) { for (const Token *tok = function->bodyStart; tok != function->bodyEnd; tok = tok->next()) { // Parse range-based for loop @@ -2824,8 +2830,12 @@ void CheckStl::useStlAlgorithm() algo = "std::count_if"; else if (accumulateBoolLiteral(assignTok, assignVarId)) algo = "std::any_of, std::all_of, std::none_of, or std::accumulate"; - else + else if (assignTok->str() != "=") algo = "std::accumulate"; + else if (isConditionWithoutSideEffects(condBodyTok)) + algo = "std::any_of, std::all_of, std::none_of"; + else + continue; } useStlAlgorithmError(assignTok, algo); continue; diff --git a/lib/reverseanalyzer.cpp b/lib/reverseanalyzer.cpp index c6c86c758..31f27a768 100644 --- a/lib/reverseanalyzer.cpp +++ b/lib/reverseanalyzer.cpp @@ -244,7 +244,7 @@ struct ReverseTraversal { // Assignment to } else if (lhsAction.matches() && !assignTok->astOperand2()->hasKnownIntValue() && assignTok->astOperand2()->exprId() > 0 && - isConstExpression(assignTok->astOperand2(), settings->library, true, true)) { + isConstExpression(assignTok->astOperand2(), settings->library, true)) { const std::string info = "Assignment to '" + assignTok->expressionString() + "'"; ValuePtr a = analyzer->reanalyze(assignTok->astOperand2(), info); if (a) { diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 15f7226b2..5eb56fc9c 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -5039,7 +5039,7 @@ static void valueFlowConditionExpressions(TokenList *tokenlist, SymbolDatabase* continue; if (condTok->hasKnownIntValue()) continue; - if (!isConstExpression(condTok, settings->library, true, tokenlist->isCPP())) + if (!isConstExpression(condTok, settings->library, tokenlist->isCPP())) continue; const bool is1 = (condTok->isComparisonOp() || condTok->tokType() == Token::eLogicalOp || astIsBool(condTok)); @@ -5166,7 +5166,7 @@ static void valueFlowSymbolic(TokenList* tokenlist, SymbolDatabase* symboldataba continue; if (tok->astOperand2()->exprId() == 0) continue; - if (!isConstExpression(tok->astOperand2(), tokenlist->getSettings()->library, true, tokenlist->isCPP())) + if (!isConstExpression(tok->astOperand2(), tokenlist->getSettings()->library, tokenlist->isCPP())) continue; if (tok->astOperand1()->valueType() && tok->astOperand2()->valueType()) { if (isTruncated( @@ -5918,7 +5918,7 @@ struct ConditionHandler { continue; if (cond.true_values.empty() || cond.false_values.empty()) continue; - if (!isConstExpression(cond.vartok, tokenlist->getSettings()->library, true, tokenlist->isCPP())) + if (!isConstExpression(cond.vartok, tokenlist->getSettings()->library, tokenlist->isCPP())) continue; f(cond, tok, scope); } @@ -6625,7 +6625,7 @@ struct SymbolicConditionHandler : SimpleConditionHandler { return {}; if (!tok->astOperand2() || tok->astOperand2()->hasKnownIntValue() || tok->astOperand2()->isLiteral()) return {}; - if (!isConstExpression(tok, settings->library, true, true)) + if (!isConstExpression(tok, settings->library, true)) return {}; std::vector result; diff --git a/test/teststl.cpp b/test/teststl.cpp index d0d9292f3..5bbaf39b4 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -5200,6 +5200,25 @@ private: "}\n", true); ASSERT_EQUALS("", errout.str()); + + check("bool g(int);\n" + "int f(const std::vector& v) {\n" + " int ret = 0;\n" + " for (const auto i : v)\n" + " if (!g(i))\n" + " ret = 1;\n" + " return ret;\n" + "}\n"); + ASSERT_EQUALS("", errout.str()); + + check("int f(const std::vector& v) {\n" + " int ret = 0;\n" + " for (const auto i : v)\n" + " if (i < 5)\n" + " ret = 1;\n" + " return ret;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:5]: (style) Consider using std::any_of, std::all_of, std::none_of algorithm instead of a raw loop.\n", errout.str()); } void loopAlgoMinMax() {