From 68c129d74da9e0651fdc9ac83f1ab73c5d5485c0 Mon Sep 17 00:00:00 2001 From: chrchr-github <78114321+chrchr-github@users.noreply.github.com> Date: Sun, 2 Mar 2025 09:46:59 +0100 Subject: [PATCH] Fix #11493 FN useStlAlgorithm with indices (#7317) --- .selfcheck_suppressions | 1 + gui/projectfiledialog.cpp | 9 ++-- gui/resultstree.cpp | 10 ++-- gui/translationhandler.cpp | 12 ++--- lib/checkstl.cpp | 99 ++++++++++++++++++++++++++++++++------ lib/clangimport.cpp | 15 +++--- lib/tokenize.cpp | 9 ++-- lib/tokenlist.cpp | 8 +-- test/teststl.cpp | 79 +++++++++++++++++++++++++++++- 9 files changed, 188 insertions(+), 54 deletions(-) diff --git a/.selfcheck_suppressions b/.selfcheck_suppressions index ec4db9fa7e1..c3930ea9bcf 100644 --- a/.selfcheck_suppressions +++ b/.selfcheck_suppressions @@ -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 diff --git a/gui/projectfiledialog.cpp b/gui/projectfiledialog.cpp index c4e6d243ef4..ef10b0ddd51 100644 --- a/gui/projectfiledialog.cpp +++ b/gui/projectfiledialog.cpp @@ -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(std::distance(mSuppressions.cbegin(), it)); } void ProjectFileDialog::browseMisraFile() diff --git a/gui/resultstree.cpp b/gui/resultstree.cpp index 247ff920742..bbce48c2a6a 100644 --- a/gui/resultstree.cpp +++ b/gui/resultstree.cpp @@ -1290,12 +1290,10 @@ void ResultsTree::saveErrors(Report *report, const QStandardItem *fileItem) cons static int indexOf(const QList &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(std::distance(list.cbegin(), it)); } void ResultsTree::updateFromOldReport(const QString &filename) diff --git a/gui/translationhandler.cpp b/gui/translationhandler.cpp index c41ccbfea96..758c873d2bb 100644 --- a/gui/translationhandler.cpp +++ b/gui/translationhandler.cpp @@ -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(std::distance(mTranslations.cbegin(), it)); } diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index ba473467515..6d09072f80e 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -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) @@ -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; } @@ -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; @@ -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; } @@ -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, "<|<=")) @@ -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; @@ -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); @@ -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; @@ -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 @@ -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()) @@ -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; @@ -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"; diff --git a/lib/clangimport.cpp b/lib/clangimport.cpp index afb56958f13..e14debcd51d 100644 --- a/lib/clangimport.cpp +++ b/lib/clangimport.cpp @@ -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, ")"); diff --git a/lib/tokenize.cpp b/lib/tokenize.cpp index 4dc76e22e1b..6ac8fcf58d8 100644 --- a/lib/tokenize.cpp +++ b/lib/tokenize.cpp @@ -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, ">")) { diff --git a/lib/tokenlist.cpp b/lib/tokenlist.cpp index b9c3ab07f04..95d11a8aa25 100644 --- a/lib/tokenlist.cpp +++ b/lib/tokenlist.cpp @@ -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(std::distance(mFiles.cbegin(), it)); // The "mFiles" vector remembers what files have been tokenized.. mFiles.push_back(std::move(fileName)); diff --git a/test/teststl.cpp b/test/teststl.cpp index 35ede750630..f7bcd312d50 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -606,7 +606,7 @@ class TestStl : public TestFixture { " return i;\n" " return 0;\n" "}\n"); - ASSERT_EQUALS("", errout_str()); + ASSERT_EQUALS("test.cpp:8:style:Consider using std::find_if algorithm instead of a raw loop.\n", errout_str()); checkNormal("bool g();\n" "int f(int x) {\n" @@ -619,7 +619,7 @@ class TestStl : public TestFixture { " return i;\n" " return 0;\n" "}\n"); - ASSERT_EQUALS("", errout_str()); + ASSERT_EQUALS("test.cpp:8:style:Consider using std::find_if algorithm instead of a raw loop.\n", errout_str()); checkNormal("bool g();\n" "void f(int x) {\n" @@ -5439,6 +5439,32 @@ class TestStl : public TestFixture { " return x;\n" "}\n"); ASSERT_EQUALS("", errout_str()); + + check("int f(const std::vector& v) {\n" // #11493 + " int s = 0;\n" + " for (std::size_t i = 0; i < v.size(); ++i)\n" + " s += v[i];\n" + " return s;\n" + "}\n"); + ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::accumulate algorithm instead of a raw loop.\n", errout_str()); + + check("int f(int n) {\n" + " int s = 0;\n" + " for (int i = 0; i < n; ++i)\n" + " s += g(i);\n" + " return s;\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + check("int g(int);\n" + "int f(const std::vector&v, int n) {\n" + " int s = 0;\n" + " for (int i = 0; i < n; ++i) {\n" + " s += g(i) + v[i];\n" + " }\n" + " return s;\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); } void loopAlgoContainerInsert() { @@ -5816,6 +5842,35 @@ class TestStl : public TestFixture { " throw 1;\n" "}\n"); ASSERT_EQUALS("", errout_str()); + + check("bool f(const std::vector& v, const std::vector& w, int n) {\n" + " for (int i = 0; i < n; ++i)\n" + " if (v[i] == w[i])\n" + " return true;\n" + " return false;\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + check("bool f(int n) {\n" + " for (int i = 0; i < n; ++i)\n" + " if (g(i))\n" + " return true;\n" + " return false;\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); + + check("bool g(int);\n" + "bool f(const std::vector&v, int n) {\n" + " bool b{};\n" + " for (int i = 0; i < n; ++i) {\n" + " if (v[i] > 0 && g(i)) {\n" + " b = true;\n" + " break;\n" + " }\n" + " }\n" + " return b;\n" + "}\n"); + ASSERT_EQUALS("", errout_str()); } void loopAlgoMinMax() { @@ -5879,6 +5934,26 @@ class TestStl : public TestFixture { "}\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()); + + check("int f(const std::vector& v) {\n" + " int max = 0;\n" + " for (int i = 0; i < n; ++i)\n" + " max = v[i] > max ? v[i] : max;\n" + " return max;\n" + "}\n", + dinit(CheckOptions, $.inconclusive = true)); + ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::max_element algorithm instead of a raw loop.\n", + errout_str()); + + check("int f(const std::vector& v) {\n" + " int min = 0;\n" + " for (int i = 0; i < n; ++i)\n" + " min = v[i] < min ? v[i] : min;\n" + " return min;\n" + "}\n", + dinit(CheckOptions, $.inconclusive = true)); + ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::min_element algorithm instead of a raw loop.\n", + errout_str()); } void loopAlgoMultipleReturn()