From 9351eddbcaf51ae51f78f4766c5eb09dfd23e5f4 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Thu, 9 Mar 2023 10:06:27 -0600 Subject: [PATCH] Fix 11605: FN useStlAlgo with multiple conditions (#4873) --- lib/astutils.cpp | 11 +-- lib/checkother.cpp | 6 +- lib/checkstl.cpp | 154 +++++++++++++++++++++++++++++++++++++++-- lib/symboldatabase.cpp | 6 +- test/teststl.cpp | 116 +++++++++++++++++++++++++++++-- test/testvalueflow.cpp | 40 +++++++---- 6 files changed, 297 insertions(+), 36 deletions(-) diff --git a/lib/astutils.cpp b/lib/astutils.cpp index 9dcf6f4b2..327ea5932 100644 --- a/lib/astutils.cpp +++ b/lib/astutils.cpp @@ -1981,17 +1981,18 @@ bool isUniqueExpression(const Token* tok) if (!scope) return true; const std::string returnType = fun->retType ? fun->retType->name() : fun->retDef->stringifyList(fun->tokenDef); - for (const Function& f:scope->functionList) { + if (!std::all_of(scope->functionList.begin(), scope->functionList.end(), [&](const Function& f) { if (f.type != Function::eFunction) - continue; + return true; const std::string freturnType = f.retType ? f.retType->name() : f.retDef->stringifyList(f.returnDefEnd()); - if (f.argumentList.size() == fun->argumentList.size() && - returnType == freturnType && + if (f.argumentList.size() == fun->argumentList.size() && returnType == freturnType && f.name() != fun->name()) { return false; } - } + return true; + })) + return false; } else if (tok->variable()) { const Variable * var = tok->variable(); const Scope * scope = var->scope(); diff --git a/lib/checkother.cpp b/lib/checkother.cpp index d9453cc0a..ae3d33a15 100644 --- a/lib/checkother.cpp +++ b/lib/checkother.cpp @@ -2780,7 +2780,7 @@ void CheckOther::pointerPositiveError(const Token *tok, const ValueFlow::Value * /* check if a constructor in given class scope takes a reference */ static bool constructorTakesReference(const Scope * const classScope) { - for (const Function &constructor : classScope->functionList) { + return std::any_of(classScope->functionList.begin(), classScope->functionList.end(), [&](const Function& constructor) { if (constructor.isConstructor()) { for (int argnr = 0U; argnr < constructor.argCount(); argnr++) { const Variable * const argVar = constructor.getArgumentVar(argnr); @@ -2789,8 +2789,8 @@ static bool constructorTakesReference(const Scope * const classScope) } } } - } - return false; + return false; + }); } //--------------------------------------------------------------------------- diff --git a/lib/checkstl.cpp b/lib/checkstl.cpp index 027f22f8c..ba12f4664 100644 --- a/lib/checkstl.cpp +++ b/lib/checkstl.cpp @@ -2670,6 +2670,142 @@ static std::string minmaxCompare(const Token *condTok, nonneg int loopVar, nonne return algo; } +namespace { + struct LoopAnalyzer { + const Token* bodyTok = nullptr; + const Token* loopVar = nullptr; + const Settings* settings = nullptr; + std::set varsChanged = {}; + + explicit LoopAnalyzer(const Token* tok, const Settings* psettings) + : bodyTok(tok->next()->link()->next()), settings(psettings) + { + const Token* splitTok = tok->next()->astOperand2(); + if (Token::simpleMatch(splitTok, ":") && splitTok->previous()->varId() != 0) { + loopVar = splitTok->previous(); + } + if (valid()) { + findChangedVariables(); + } + } + bool isLoopVarChanged() const { + return varsChanged.count(loopVar->varId()) > 0; + } + + bool isModified(const Token* tok) const + { + if (tok->variable() && tok->variable()->isConst()) + return false; + int n = 1 + (astIsPointer(tok) ? 1 : 0); + for (int i = 0; i < n; i++) { + bool inconclusive = false; + if (isVariableChangedByFunctionCall(tok, i, settings, &inconclusive)) + return true; + if (inconclusive) + return true; + if (isVariableChanged(tok, i, settings, true)) + return true; + } + return false; + } + + template + void findTokens(Predicate pred, F f) const + { + for (const Token* tok = bodyTok; precedes(tok, bodyTok->link()); tok = tok->next()) { + if (pred(tok)) + f(tok); + } + } + + template + const Token* findToken(Predicate pred) const + { + for (const Token* tok = bodyTok; precedes(tok, bodyTok->link()); tok = tok->next()) { + if (pred(tok)) + return tok; + } + return nullptr; + } + + bool hasGotoOrBreak() const + { + return findToken([](const Token* tok) { + return Token::Match(tok, "goto|break"); + }); + } + + bool valid() const { + return bodyTok && loopVar; + } + + std::string findAlgo() const + { + if (!valid()) + return ""; + bool loopVarChanged = isLoopVarChanged(); + if (!loopVarChanged && varsChanged.empty()) { + if (hasGotoOrBreak()) + return ""; + bool alwaysTrue = true; + bool alwaysFalse = true; + auto hasReturn = [](const Token* tok) { + return Token::simpleMatch(tok, "return"); + }; + findTokens(hasReturn, [&](const Token* tok) { + const Token* returnTok = tok->astOperand1(); + if (!returnTok || !returnTok->hasKnownIntValue() || !astIsBool(returnTok)) { + alwaysTrue = false; + alwaysFalse = false; + return; + } + (returnTok->values().front().intvalue ? alwaysTrue : alwaysFalse) &= true; + (returnTok->values().front().intvalue ? alwaysFalse : alwaysTrue) &= false; + }); + if (alwaysTrue == alwaysFalse) + return ""; + if (alwaysTrue) + return "std::any_of"; + else + return "std::all_of or std::none_of"; + } + return ""; + } + + bool isLocalVar(const Variable* var) const + { + if (!var) + return false; + if (var->isPointer() || var->isReference()) + return false; + if (var->declarationId() == loopVar->varId()) + return false; + const Scope* scope = var->scope(); + return scope->isNestedIn(bodyTok->scope()); + } + + private: + void findChangedVariables() + { + std::set vars; + for (const Token* tok = bodyTok; precedes(tok, bodyTok->link()); tok = tok->next()) { + if (tok->varId() == 0) + continue; + if (vars.count(tok->varId()) > 0) + continue; + if (isLocalVar(tok->variable())) { + vars.insert(tok->varId()); + continue; + } + if (!isModified(tok)) + continue; + varsChanged.insert(tok->varId()); + vars.insert(tok->varId()); + } + } + }; +} // namespace + void CheckStl::useStlAlgorithm() { if (!mSettings->severity.isEnabled(Severity::style)) @@ -2694,6 +2830,13 @@ void CheckStl::useStlAlgorithm() continue; if (!Token::simpleMatch(tok->next()->link(), ") {")) continue; + LoopAnalyzer a{tok, mSettings}; + std::string algoName = a.findAlgo(); + if (!algoName.empty()) { + useStlAlgorithmError(tok, algoName); + continue; + } + const Token *bodyTok = tok->next()->link()->next(); const Token *splitTok = tok->next()->astOperand2(); const Token* loopVar{}; @@ -2880,16 +3023,15 @@ static bool isKnownEmptyContainer(const Token* tok) { if (!tok) return false; - for (const ValueFlow::Value& v:tok->values()) { + return std::any_of(tok->values().begin(), tok->values().end(), [&](const ValueFlow::Value& v) { if (!v.isKnown()) - continue; + return false; if (!v.isContainerSizeValue()) - continue; + return false; if (v.intvalue != 0) - continue; + return false; return true; - } - return false; + }); } void CheckStl::knownEmptyContainer() diff --git a/lib/symboldatabase.cpp b/lib/symboldatabase.cpp index 2dc897704..50c6b7521 100644 --- a/lib/symboldatabase.cpp +++ b/lib/symboldatabase.cpp @@ -5128,7 +5128,7 @@ const Type* SymbolDatabase::findVariableType(const Scope *start, const Token *ty bool Scope::hasInlineOrLambdaFunction() const { - for (const Scope *s : nestedList) { + return std::any_of(nestedList.begin(), nestedList.end(), [&](const Scope* s) { // Inline function if (s->type == Scope::eUnconditional && Token::simpleMatch(s->bodyStart->previous(), ") {")) return true; @@ -5137,8 +5137,8 @@ bool Scope::hasInlineOrLambdaFunction() const return true; if (s->hasInlineOrLambdaFunction()) return true; - } - return false; + return false; + }); } void Scope::findFunctionInBase(const std::string & name, nonneg int args, std::vector & matches) const diff --git a/test/teststl.cpp b/test/teststl.cpp index c519c184c..d4a9cce16 100644 --- a/test/teststl.cpp +++ b/test/teststl.cpp @@ -170,6 +170,7 @@ private: TEST_CASE(loopAlgoIncrement); TEST_CASE(loopAlgoConditional); TEST_CASE(loopAlgoMinMax); + TEST_CASE(loopAlgoMultipleReturn); TEST_CASE(invalidContainer); TEST_CASE(invalidContainerLoop); @@ -574,7 +575,7 @@ private: " return true;\n" " return false;\n" "}\n"); - ASSERT_EQUALS("test.cpp:6:style:Consider using std::any_of algorithm instead of a raw loop.\n", errout.str()); + ASSERT_EQUALS("test.cpp:5:style:Consider using std::any_of algorithm instead of a raw loop.\n", errout.str()); checkNormal("std::vector range(int n);\n" "bool f(bool b) {\n" @@ -587,7 +588,7 @@ private: " return true;\n" " return false;\n" "}\n"); - ASSERT_EQUALS("test.cpp:8:style:Consider using std::any_of algorithm instead of a raw loop.\n", errout.str()); + ASSERT_EQUALS("test.cpp:7:style:Consider using std::any_of algorithm instead of a raw loop.\n", errout.str()); checkNormal("bool g();\n" "int f(int x) {\n" @@ -5120,7 +5121,8 @@ private: " return true;\n" "}\n", true); - ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::any_of algorithm instead of a raw loop.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::all_of or std::none_of algorithm instead of a raw loop.\n", + errout.str()); check("bool pred(int x);\n" "bool foo() {\n" @@ -5293,7 +5295,8 @@ private: " }\n" " return false;\n" "}\n"); - ASSERT_EQUALS("[test.cpp:3]: (style) Consider using std::any_of algorithm instead of a raw loop.\n", errout.str()); + ASSERT_EQUALS("[test.cpp:2]: (style) Consider using std::any_of algorithm instead of a raw loop.\n", + errout.str()); } void loopAlgoMinMax() { @@ -5338,6 +5341,111 @@ private: ASSERT_EQUALS("[test.cpp:4]: (style) Consider using std::accumulate algorithm instead of a raw loop.\n", errout.str()); } + void loopAlgoMultipleReturn() + { + check("bool f(const std::vector& v) {\n" + " for (auto i : v) {\n" + " if (i < 0)\n" + " continue;\n" + " if (i)\n" + " return true;\n" + " }\n" + " return false;\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:2]: (style) Consider using std::any_of algorithm instead of a raw loop.\n", + errout.str()); + + check("bool g(const std::vector& v) {\n" + " for (auto i : v) {\n" + " if (i % 5 == 0)\n" + " return true;\n" + " if (i % 7 == 0)\n" + " return true;\n" + " }\n" + " return false;\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:2]: (style) Consider using std::any_of algorithm instead of a raw loop.\n", + errout.str()); + + check("bool g(const std::vector& v) {\n" + " for (auto i : v) {\n" + " if (i % 5 == 0)\n" + " return false;\n" + " if (i % 7 == 0)\n" + " return true;\n" + " }\n" + " return false;\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("bool g(std::vector& v) {\n" + " for (auto& i : v) {\n" + " if (i % 5 == 0)\n" + " return false;\n" + " if (i % 7 == 0)\n" + " i++;\n" + " }\n" + " return false;\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("bool g(const std::vector& v, int& j) {\n" + " for (auto i : v) {\n" + " if (i % 5 == 0)\n" + " return false;\n" + " if (i % 7 == 0)\n" + " j++;\n" + " }\n" + " return false;\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("bool g(const std::vector& v, int& j) {\n" + " for (auto i : v) {\n" + " int& k = j;\n" + " if (i % 5 == 0)\n" + " return false;\n" + " if (i % 7 == 0)\n" + " k++;\n" + " }\n" + " return false;\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("bool g(const std::vector& v, int& j) {\n" + " for (auto i : v) {\n" + " int* k = &j;\n" + " if (i % 5 == 0)\n" + " return false;\n" + " if (i % 7 == 0)\n" + " (*k)++;\n" + " }\n" + " return false;\n" + "}\n", + true); + ASSERT_EQUALS("", errout.str()); + + check("bool g(const std::vector& v, int j) {\n" + " for (auto i : v) {\n" + " int k = j;\n" + " if (i % 5 == 0)\n" + " return false;\n" + " if (i % 7 == 0)\n" + " k++;\n" + " }\n" + " return false;\n" + "}\n", + true); + ASSERT_EQUALS("[test.cpp:2]: (style) Consider using std::all_of or std::none_of algorithm instead of a raw loop.\n", + errout.str()); + } + void invalidContainer() { check("void f(std::vector &v) {\n" " auto v0 = v.begin();\n" diff --git a/test/testvalueflow.cpp b/test/testvalueflow.cpp index e1ff1a95c..82d1735d6 100644 --- a/test/testvalueflow.cpp +++ b/test/testvalueflow.cpp @@ -202,12 +202,14 @@ private: for (const Token *tok = tokenizer.tokens(); tok; tok = tok->next()) { if (tok->str() == "x" && tok->linenr() == linenr) { - for (const ValueFlow::Value& val:tok->values()) { + if (std::any_of(tok->values().begin(), tok->values().end(), [&](const ValueFlow::Value& val) { if (val.isSymbolicValue()) - continue; + return false; if (val.isKnown() && val.intvalue == value) return true; - } + return false; + })) + return true; } } @@ -222,12 +224,14 @@ private: for (const Token* tok = tokenizer.tokens(); tok; tok = tok->next()) { if (tok->str() == "x" && tok->linenr() == linenr) { - for (const ValueFlow::Value& val : tok->values()) { + if (std::any_of(tok->values().begin(), tok->values().end(), [&](const ValueFlow::Value& val) { if (!val.isSymbolicValue()) - continue; + return false; if (val.isKnown() && val.intvalue == value && val.tokvalue->expressionString() == expr) return true; - } + return false; + })) + return true; } } @@ -243,12 +247,14 @@ private: for (const Token *tok = tokenizer.tokens(); tok; tok = tok->next()) { if (tok->str() == "x" && tok->linenr() == linenr) { - for (const ValueFlow::Value& val:tok->values()) { + if (std::any_of(tok->values().begin(), tok->values().end(), [&](const ValueFlow::Value& val) { if (val.isSymbolicValue()) - continue; + return false; if (val.isImpossible() && val.intvalue == value) return true; - } + return false; + })) + return true; } } @@ -264,12 +270,14 @@ private: for (const Token* tok = tokenizer.tokens(); tok; tok = tok->next()) { if (tok->str() == "x" && tok->linenr() == linenr) { - for (const ValueFlow::Value& val : tok->values()) { + if (std::any_of(tok->values().begin(), tok->values().end(), [&](const ValueFlow::Value& val) { if (!val.isSymbolicValue()) - continue; + return false; if (val.isImpossible() && val.intvalue == value && val.tokvalue->expressionString() == expr) return true; - } + return false; + })) + return true; } } @@ -285,12 +293,14 @@ private: for (const Token *tok = tokenizer.tokens(); tok; tok = tok->next()) { if (tok->str() == "x" && tok->linenr() == linenr) { - for (const ValueFlow::Value& val:tok->values()) { + if (std::any_of(tok->values().begin(), tok->values().end(), [&](const ValueFlow::Value& val) { if (val.isSymbolicValue()) - continue; + return false; if (val.isInconclusive() && val.intvalue == value) return true; - } + return false; + })) + return true; } }