Skip to content

Commit

Permalink
Fix #11493 FN useStlAlgorithm with indices (danmar#7317)
Browse files Browse the repository at this point in the history
  • Loading branch information
chrchr-github authored Mar 2, 2025
1 parent 1852b10 commit 68c129d
Show file tree
Hide file tree
Showing 9 changed files with 188 additions and 54 deletions.
1 change: 1 addition & 0 deletions .selfcheck_suppressions
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,4 @@ invalidPrintfArgType_uint:externals/tinyxml2/tinyxml2.cpp
funcArgNamesDifferent:externals/tinyxml2/tinyxml2.cpp
nullPointerRedundantCheck:externals/tinyxml2/tinyxml2.cpp
knownConditionTrueFalse:externals/tinyxml2/tinyxml2.cpp
useStlAlgorithm:externals/simplecpp/simplecpp.cpp
9 changes: 4 additions & 5 deletions gui/projectfiledialog.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -945,11 +945,10 @@ void ProjectFileDialog::editSuppression(const QModelIndex & /*index*/)
int ProjectFileDialog::getSuppressionIndex(const QString &shortText) const
{
const std::string s = shortText.toStdString();
for (int i = 0; i < mSuppressions.size(); ++i) {
if (mSuppressions[i].getText() == s)
return i;
}
return -1;
auto it = std::find_if(mSuppressions.cbegin(), mSuppressions.cend(), [&](const SuppressionList::Suppression& sup) {
return sup.getText() == s;
});
return it == mSuppressions.cend() ? -1 : static_cast<int>(std::distance(mSuppressions.cbegin(), it));
}

void ProjectFileDialog::browseMisraFile()
Expand Down
10 changes: 4 additions & 6 deletions gui/resultstree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1290,12 +1290,10 @@ void ResultsTree::saveErrors(Report *report, const QStandardItem *fileItem) cons

static int indexOf(const QList<ErrorItem> &list, const ErrorItem &item)
{
for (int i = 0; i < list.size(); i++) {
if (ErrorItem::sameCID(item, list[i])) {
return i;
}
}
return -1;
auto it = std::find_if(list.cbegin(), list.cend(), [&](const ErrorItem& e) {
return ErrorItem::sameCID(item, e);
});
return it == list.cend() ? -1 : static_cast<int>(std::distance(list.cbegin(), it));
}

void ResultsTree::updateFromOldReport(const QString &filename)
Expand Down
12 changes: 4 additions & 8 deletions gui/translationhandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,8 @@ void TranslationHandler::addTranslation(const char *name, const char *filename)

int TranslationHandler::getLanguageIndexByCode(const QString &code) const
{
int index = -1;
for (int i = 0; i < mTranslations.size(); i++) {
if (mTranslations[i].mCode == code || mTranslations[i].mCode == code.left(2)) {
index = i;
break;
}
}
return index;
auto it = std::find_if(mTranslations.cbegin(), mTranslations.cend(), [&](const TranslationInfo& ti) {
return ti.mCode == code || ti.mCode == code.left(2);
});
return it == mTranslations.cend() ? -1 : static_cast<int>(std::distance(mTranslations.cbegin(), it));
}
99 changes: 83 additions & 16 deletions lib/checkstl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2598,7 +2598,9 @@ static const Token *singleStatement(const Token *start)
return endStatement;
}

static const Token *singleAssignInScope(const Token *start, nonneg int varid, bool &input, bool &hasBreak, const Settings& settings)
enum class LoopType : std::uint8_t { OTHER, RANGE, ITERATOR, INDEX };

static const Token *singleAssignInScope(const Token *start, nonneg int varid, bool &input, bool &hasBreak, LoopType loopType, const Settings& settings)
{
const Token *endStatement = singleStatement(start);
if (!endStatement)
Expand All @@ -2612,6 +2614,23 @@ static const Token *singleAssignInScope(const Token *start, nonneg int varid, bo
return nullptr;
input = Token::findmatch(assignTok->next(), "%varid%", endStatement, varid) || !Token::Match(start->next(), "%var% =");
hasBreak = Token::simpleMatch(endStatement->previous(), "break");

if (loopType == LoopType::INDEX) { // check for container access
nonneg int containerId{};
for (const Token* tok = assignTok->next(); tok != endStatement; tok = tok->next()) {
if (tok->varId() == varid) {
if (!Token::simpleMatch(tok->astParent(), "["))
return nullptr;
const Token* contTok = tok->astParent()->astOperand1();
if (!contTok->valueType() || !contTok->valueType()->container || contTok->varId() == 0)
return nullptr;
if (containerId > 0 && containerId != contTok->varId()) // allow only one container
return nullptr;
containerId = contTok->varId();
}
}
return containerId > 0 ? assignTok : nullptr;
}
return assignTok;
}

Expand Down Expand Up @@ -2652,7 +2671,7 @@ static const Token *singleIncrementInScope(const Token *start, nonneg int varid,
return varTok;
}

static const Token *singleConditionalInScope(const Token *start, nonneg int varid, const Settings& settings)
static const Token *singleConditionalInScope(const Token *start, nonneg int varid, LoopType loopType, const Settings& settings)
{
if (start->str() != "{")
return nullptr;
Expand All @@ -2671,6 +2690,22 @@ static const Token *singleConditionalInScope(const Token *start, nonneg int vari
return nullptr;
if (isVariableChanged(start, bodyTok, varid, /*globalvar*/ false, settings))
return nullptr;
if (loopType == LoopType::INDEX) { // check for container access
nonneg int containerId{};
for (const Token* tok = start->tokAt(2); tok != start->linkAt(2); tok = tok->next()) {
if (tok->varId() == varid) {
if (!Token::simpleMatch(tok->astParent(), "["))
return nullptr;
const Token* contTok = tok->astParent()->astOperand1();
if (!contTok->valueType() || !contTok->valueType()->container || contTok->varId() == 0)
return nullptr;
if (containerId > 0 && containerId != contTok->varId()) // allow only one container
return nullptr;
containerId = contTok->varId();
}
}
return containerId > 0 ? bodyTok : nullptr;
}
return bodyTok;
}

Expand Down Expand Up @@ -2735,11 +2770,9 @@ static std::string flipMinMax(const std::string &algo)
return algo;
}

static std::string minmaxCompare(const Token *condTok, nonneg int loopVar, nonneg int assignVar, bool invert = false)
static std::string minmaxCompare(const Token *condTok, nonneg int loopVar, nonneg int assignVar, LoopType loopType, bool invert = false)
{
if (!Token::Match(condTok, "<|<=|>=|>"))
return "std::accumulate";
if (!hasVarIds(condTok, loopVar, assignVar))
if (loopType == LoopType::RANGE && !hasVarIds(condTok, loopVar, assignVar))
return "std::accumulate";
std::string algo = "std::max_element";
if (Token::Match(condTok, "<|<="))
Expand All @@ -2751,6 +2784,38 @@ static std::string minmaxCompare(const Token *condTok, nonneg int loopVar, nonne
return algo;
}

static bool isTernaryAssignment(const Token* assignTok, nonneg int loopVarId, nonneg int assignVarId, LoopType loopType, std::string& algo)
{
if (!Token::simpleMatch(assignTok->astOperand2(), "?"))
return false;
const Token* condTok = assignTok->astOperand2()->astOperand1();
if (!Token::Match(condTok, "<|<=|>=|>"))
return false;

const Token* colon = assignTok->astOperand2()->astOperand2();
if (loopType == LoopType::RANGE) {
if (!(condTok->astOperand1()->varId() && condTok->astOperand2()->varId() && colon->astOperand1()->varId() && colon->astOperand2()->varId()))
return false;
}
else if (loopType == LoopType::INDEX) {
int nVar = 0, nCont = 0;
for (const Token* tok : { condTok->astOperand1(), condTok->astOperand2(), colon->astOperand1(), colon->astOperand2() }) {
if (tok->varId())
++nVar;
else if (tok->str() == "[" && tok->astOperand1()->varId() && tok->astOperand1()->valueType() && tok->astOperand1()->valueType()->container &&
tok->astOperand2()->varId() == loopVarId)
++nCont;
}
if (nVar != 2 || nCont != 2)
return false;
}
else
return false;

algo = minmaxCompare(condTok, loopVarId, assignVarId, loopType, colon->astOperand1()->varId() == assignVarId);
return true;
}

namespace {
struct LoopAnalyzer {
const Token* bodyTok = nullptr;
Expand Down Expand Up @@ -2940,13 +3005,14 @@ void CheckStl::useStlAlgorithm()
const Token *bodyTok = tok->linkAt(1)->next();
const Token *splitTok = tok->next()->astOperand2();
const Token* loopVar{};
bool isIteratorLoop = false;
LoopType loopType{};
if (Token::simpleMatch(splitTok, ":")) {
loopVar = splitTok->previous();
if (loopVar->varId() == 0)
continue;
if (Token::simpleMatch(splitTok->astOperand2(), "{"))
continue;
loopType = LoopType::RANGE;
}
else { // iterator-based loop?
const Token* initTok = getInitTok(tok);
Expand All @@ -2955,18 +3021,19 @@ void CheckStl::useStlAlgorithm()
if (!initTok || !condTok || !stepTok)
continue;
loopVar = Token::Match(condTok, "%comp%") ? condTok->astOperand1() : nullptr;
if (!Token::Match(loopVar, "%var%") || !loopVar->valueType() || loopVar->valueType()->type != ValueType::Type::ITERATOR)
if (!Token::Match(loopVar, "%var%") || !loopVar->valueType() ||
(loopVar->valueType()->type != ValueType::Type::ITERATOR && !loopVar->valueType()->isIntegral()))
continue;
if (!Token::simpleMatch(initTok, "=") || !Token::Match(initTok->astOperand1(), "%varid%", loopVar->varId()))
continue;
if (!stepTok->isIncDecOp())
continue;
isIteratorLoop = true;
loopType = (loopVar->valueType()->type == ValueType::Type::ITERATOR) ? LoopType::ITERATOR : LoopType::INDEX;
}

// Check for single assignment
bool useLoopVarInAssign{}, hasBreak{};
const Token *assignTok = singleAssignInScope(bodyTok, loopVar->varId(), useLoopVarInAssign, hasBreak, *mSettings);
const Token *assignTok = singleAssignInScope(bodyTok, loopVar->varId(), useLoopVarInAssign, hasBreak, loopType, *mSettings);
if (assignTok) {
if (!checkAssignee(assignTok->astOperand1()))
continue;
Expand All @@ -2986,8 +3053,8 @@ void CheckStl::useStlAlgorithm()
algo = "std::distance";
else if (accumulateBool(assignTok, assignVarId))
algo = "std::any_of, std::all_of, std::none_of, or std::accumulate";
else if (Token::Match(assignTok, "= %var% <|<=|>=|> %var% ? %var% : %var%") && hasVarIds(assignTok->tokAt(6), loopVar->varId(), assignVarId))
algo = minmaxCompare(assignTok->tokAt(2), loopVar->varId(), assignVarId, assignTok->tokAt(5)->varId() == assignVarId);
else if (isTernaryAssignment(assignTok, loopVar->varId(), assignVarId, loopType, algo))
;
else if (isAccumulation(assignTok, assignVarId))
algo = "std::accumulate";
else
Expand All @@ -2999,7 +3066,7 @@ void CheckStl::useStlAlgorithm()
// Check for container calls
bool useLoopVarInMemCall;
const Token *memberAccessTok = singleMemberCallInScope(bodyTok, loopVar->varId(), useLoopVarInMemCall, *mSettings);
if (memberAccessTok && !isIteratorLoop) {
if (memberAccessTok && loopType == LoopType::RANGE) {
const Token *memberCallTok = memberAccessTok->astOperand2();
const int contVarId = memberAccessTok->astOperand1()->varId();
if (contVarId == loopVar->varId())
Expand Down Expand Up @@ -3031,10 +3098,10 @@ void CheckStl::useStlAlgorithm()
}

// Check for conditionals
const Token *condBodyTok = singleConditionalInScope(bodyTok, loopVar->varId(), *mSettings);
const Token *condBodyTok = singleConditionalInScope(bodyTok, loopVar->varId(), loopType, *mSettings);
if (condBodyTok) {
// Check for single assign
assignTok = singleAssignInScope(condBodyTok, loopVar->varId(), useLoopVarInAssign, hasBreak, *mSettings);
assignTok = singleAssignInScope(condBodyTok, loopVar->varId(), useLoopVarInAssign, hasBreak, loopType, *mSettings);
if (assignTok) {
if (!checkAssignee(assignTok->astOperand1()))
continue;
Expand Down Expand Up @@ -3108,7 +3175,7 @@ void CheckStl::useStlAlgorithm()
const Token *loopVar2 = Token::findmatch(condBodyTok, "%varid%", condBodyTok->link(), loopVar->varId());
std::string algo;
if (loopVar2 ||
(isIteratorLoop && loopVar->variable() && precedes(loopVar->variable()->nameToken(), tok))) // iterator declared outside the loop
(loopType == LoopType::ITERATOR && loopVar->variable() && precedes(loopVar->variable()->nameToken(), tok))) // iterator declared outside the loop
algo = "std::find_if";
else
algo = "std::any_of";
Expand Down
15 changes: 7 additions & 8 deletions lib/clangimport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -906,15 +906,14 @@ Token *clangimport::AstNode::createTokens(TokenList &tokenList)
varDecl->children.clear();
Token *expr1 = varDecl->createTokens(tokenList);
Token *colon = addtoken(tokenList, ":");
AstNodePtr range;
for (std::size_t i = 0; i < 2; i++) {
if (children[i] && children[i]->nodeType == DeclStmt && children[i]->getChild(0)->nodeType == VarDecl) {
range = children[i]->getChild(0)->getChild(0);
break;
}
}
if (!range)

auto it = std::find_if(children.cbegin(), children.cbegin() + 2, [&](const AstNodePtr& c) {
return c && c->nodeType == DeclStmt && c->getChild(0)->nodeType == VarDecl;
});
if (it == children.cbegin() + 2)
throw InternalError(tokenList.back(), "Failed to import CXXForRangeStmt. Range?");
AstNodePtr range = (*it)->getChild(0)->getChild(0);

Token *expr2 = range->createTokens(tokenList);
Token *par2 = addtoken(tokenList, ")");

Expand Down
9 changes: 3 additions & 6 deletions lib/tokenize.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7463,12 +7463,9 @@ void Tokenizer::simplifyStaticConst()
Token* leftTok = tok;
bool behindOther = false;
for (; leftTok; leftTok = leftTok->previous()) {
for (std::size_t j = 0; j <= i; j++) {
if (leftTok->str() == qualifiers[j]) {
behindOther = true;
break;
}
}
behindOther = std::any_of(qualifiers.cbegin(), qualifiers.cbegin() + i + 1, [&](const std::string& q) {
return q == leftTok->str();
});
if (behindOther)
break;
if (isCPP() && Token::simpleMatch(leftTok, ">")) {
Expand Down
8 changes: 5 additions & 3 deletions lib/tokenlist.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,11 @@ void TokenList::determineCppC()
int TokenList::appendFileIfNew(std::string fileName)
{
// Has this file been tokenized already?
for (int i = 0; i < mFiles.size(); ++i)
if (Path::sameFileName(mFiles[i], fileName))
return i;
auto it = std::find_if(mFiles.cbegin(), mFiles.cend(), [&](const std::string& f) {
return Path::sameFileName(f, fileName);
});
if (it != mFiles.cend())
return static_cast<int>(std::distance(mFiles.cbegin(), it));

// The "mFiles" vector remembers what files have been tokenized..
mFiles.push_back(std::move(fileName));
Expand Down
Loading

0 comments on commit 68c129d

Please sign in to comment.