Skip to content

Fix FP useStlAlgorithm (don't suggest std::accumulate when nothing is accumulated) #4647

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

Merged
danmar merged 7 commits into from
Dec 18, 2022
Merged

Fix FP useStlAlgorithm (don't suggest std::accumulate when nothing is accumulated) #4647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ded037f
Fix FP useStlAlgorithm
chrchr-github Dec 15, 2022
b687633
Improve test
chrchr-github Dec 15, 2022
fa13c04
Format
chrchr-github Dec 15, 2022
368fa80
Format
chrchr-github Dec 15, 2022
ffaff0a
Remove unused parameter
chrchr-github Dec 15, 2022
b73aeb6
Suggest std::any_of... for constant condition
chrchr-github Dec 15, 2022
78dd2ad
Format
chrchr-github Dec 15, 2022
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
4 changes: 2 additions & 2 deletions lib/astutils.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1904,7 +1904,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;
Expand All @@ -1923,7 +1923,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)
Expand Down
2 changes: 1 addition & 1 deletion lib/astutils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,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);

Expand Down
2 changes: 1 addition & 1 deletion lib/checkbool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions lib/checkfunctions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
2 changes: 1 addition & 1 deletion lib/checkother.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2520,7 +2520,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) &&
Expand Down
12 changes: 11 additions & 1 deletion lib/checkstl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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() != "=")

@pfultz2 pfultz2 Dec 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be moved up to the if above, so it is if (accumulateBoolLiteral(assignTok, assignVarId) || 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;
Expand Down
2 changes: 1 addition & 1 deletion lib/reverseanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Analyzer> a = analyzer->reanalyze(assignTok->astOperand2(), info);
if (a) {
Expand Down
8 changes: 4 additions & 4 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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<Condition> result;
Expand Down
19 changes: 19 additions & 0 deletions test/teststl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5200,6 +5200,25 @@ class TestStl : public TestFixture {
"}\n",
true);
ASSERT_EQUALS("", errout.str());

check("bool g(int);\n"
"int f(const std::vector<int>& v) {\n"
" int ret = 0;\n"
" for (const auto i : v)\n"
" if (!g(i))\n"
" ret = 1;\n"

@pfultz2 pfultz2 Dec 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this still warn when doing ret = ret + 1 or ret = h(ret)? We should probably check that the variable appears on the RHS.

@chrchr-github chrchr-github Dec 15, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We suggest std::count_if for ret = ret + 1. There is no warning for ret = h(ret) with or without this change.

" return ret;\n"
"}\n");
ASSERT_EQUALS("", errout.str());

@pfultz2 pfultz2 Dec 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should suggest std::any_of, std::all_of, or std::none_of.

@chrchr-github chrchr-github Dec 15, 2022

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, since those algorithms short-circuit. Presumably g() needs to be called for all elements.

@pfultz2 pfultz2 Dec 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it should probably be std::any_of, std::all_of, std::none_of, or std::accumulate. Then you can use std::accumulate when g() has side effects.

@pfultz2 pfultz2 Dec 15, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use isConstExpression to detect if the condition has side effects and then only suggest when there is no side effects.


check("int f(const std::vector<int>& 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() {
Expand Down