Fix 11605: FN useStlAlgo with multiple conditions (#4873)

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() &&
+            if (f.argumentList.size() == fun->argumentList.size() && returnType == freturnType &&
-                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();

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;
+    });
 }
 
 //---------------------------------------------------------------------------

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<nonneg int> 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<class Predicate, class F>
+        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<class Predicate>
+        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<nonneg int> 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()

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<const Function *> & matches) const

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<int> 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<int>& 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<int>& 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<int>& 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<int>& 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<int>& 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<int>& 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<int>& 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<int>& 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<int> &v) {\n"
               "    auto v0 = v.begin();\n"

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;
             }
         }