Refactor: ConditionHandler cleanup (#3394)

lib/valueflow.cpp

@@ -4526,29 +4526,22 @@ struct ConditionHandler {
 
     virtual std::vector<Condition> parse(const Token* tok, const Settings* settings) const = 0;
 
-    void traverseCondition(
+    void traverseCondition(TokenList* tokenlist,
-        TokenList* tokenlist,
                            SymbolDatabase* symboldatabase,
-        ErrorLogger* errorLogger,
+                           const std::function<void(const Condition& cond, Token* tok, const Scope* scope)>& f) const
-        const Settings* settings,
+    {
-        const std::function<
-            void(const Condition& cond, Token* tok, const Scope* scope, const std::vector<const Variable*>& vars)>& f) const {
         for (const Scope *scope : symboldatabase->functionScopes) {
-            std::set<nonneg int> aliased;
             for (Token *tok = const_cast<Token *>(scope->bodyStart); tok != scope->bodyEnd; tok = tok->next()) {
                 if (Token::Match(tok, "if|while|for ("))
                     continue;
 
-                if (Token::Match(tok, "= & %var% ;"))
-                    aliased.insert(tok->tokAt(2)->varId());
                 const Token* top = tok->astTop();
                 if (!top)
                     continue;
 
                 if (!Token::Match(top->previous(), "if|while|for (") && !Token::Match(tok->astParent(), "&&|%oror%|?"))
                     continue;
-                // *INDENT-OFF*
+                for (const Condition& cond : parse(tok, tokenlist->getSettings())) {
-                for (const Condition& cond : parse(tok, settings)) {
                     if (!cond.vartok)
                         continue;
                     if (cond.vartok->exprId() == 0)
@@ -4557,28 +4550,11 @@ struct ConditionHandler {
                         continue;
                     if (cond.true_values.empty() || cond.false_values.empty())
                         continue;
-                    if (!isConstExpression(cond.vartok, settings->library, true, tokenlist->isCPP()))
+                    if (!isConstExpression(cond.vartok, tokenlist->getSettings()->library, true, tokenlist->isCPP()))
-                        continue;
-                    std::vector<const Variable*> vars =
-                        getExprVariables(cond.vartok, tokenlist, symboldatabase, settings);
-                    if (std::any_of(vars.begin(), vars.end(), [](const Variable* var) { return !var; }))
-                        continue;
-                    if (!vars.empty() && (vars.front())) {
-                        if (std::any_of(vars.begin(), vars.end(), [&](const Variable* var) {
-                                return var && aliased.find(var->declarationId()) != aliased.end();
-                            })) {
-                            if (settings->debugwarnings)
-                                bailout(tokenlist,
-                                        errorLogger,
-                                        cond.vartok,
-                                        "variable is aliased so we just skip all valueflow after condition");
                         continue;
+                    f(cond, tok, scope);
                 }
             }
-                    f(cond, tok, scope, vars);
-                }
-                // *INDENT-ON*
-            }
         }
     }
 
@@ -4586,12 +4562,7 @@ struct ConditionHandler {
                          SymbolDatabase* symboldatabase,
                          ErrorLogger* errorLogger,
                          const Settings* settings) const {
-        traverseCondition(
+        traverseCondition(tokenlist, symboldatabase, [&](const Condition& cond, Token* tok, const Scope*) {
-            tokenlist,
-            symboldatabase,
-            errorLogger,
-            settings,
-            [&](const Condition& cond, Token* tok, const Scope*, const std::vector<const Variable*>&) {
             if (cond.vartok->exprId() == 0)
                 return;
 
@@ -4699,12 +4670,7 @@ struct ConditionHandler {
                         SymbolDatabase* symboldatabase,
                         ErrorLogger* errorLogger,
                         const Settings* settings) const {
-        traverseCondition(
+        traverseCondition(tokenlist, symboldatabase, [&](const Condition& cond, Token* tok, const Scope* scope) {
-            tokenlist,
-            symboldatabase,
-            errorLogger,
-            settings,
-            [&](const Condition& cond, Token* tok, const Scope* scope, const std::vector<const Variable*>& vars) {
             if (Token::simpleMatch(tok->astParent(), "?"))
                 return;
             const Token* top = tok->astTop();
@@ -4750,19 +4716,19 @@ struct ConditionHandler {
                         values = elseValues;
                     if (Token::Match(tok, "==|!=") || (tok == cond.vartok && astIsBool(tok)))
                         changePossibleToKnown(values);
-                    // *INDENT-OFF*
                     if (astIsFloat(cond.vartok, false) ||
                         (!cond.vartok->valueType() &&
                          std::all_of(values.begin(), values.end(), [](const ValueFlow::Value& v) {
                         return v.isIntValue() || v.isFloatValue();
                     })))
-                        values.remove_if([&](const ValueFlow::Value& v) { return v.isImpossible(); });
+                        values.remove_if([&](const ValueFlow::Value& v) {
+                            return v.isImpossible();
+                        });
                     for (Token* start:nextExprs) {
                         Analyzer::Result r = forward(start, cond.vartok, values, tokenlist, settings);
                         if (r.terminate != Analyzer::Terminate::None)
                             return;
                     }
-                    // *INDENT-ON*
                 }
             }
 
@@ -4792,14 +4758,6 @@ struct ConditionHandler {
             }
 
             if (top && Token::Match(top->previous(), "if|while (") && !top->previous()->isExpandedMacro()) {
-                // does condition reassign variable?
-                if (tok != top->astOperand2() && Token::Match(top->astOperand2(), "%oror%|&&") &&
-                    isVariablesChanged(top, top->link(), 0, vars, settings, tokenlist->isCPP())) {
-                    if (settings->debugwarnings)
-                        bailout(tokenlist, errorLogger, tok, "assignment in condition");
-                    return;
-                }
-
                 // if astParent is "!" we need to invert codeblock
                 {
                     const Token* tok2 = tok;
@@ -7178,10 +7136,9 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
         valueFlowSymbolicInfer(tokenlist, symboldatabase);
         valueFlowArrayBool(tokenlist);
         valueFlowRightShift(tokenlist, settings);
-        valueFlowAfterMove(tokenlist, symboldatabase, settings);
+        valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings);
         valueFlowCondition(SimpleConditionHandler{}, tokenlist, symboldatabase, errorLogger, settings);
         valueFlowInferCondition(tokenlist, settings);
-        valueFlowAfterAssign(tokenlist, symboldatabase, errorLogger, settings);
         valueFlowSwitchVariable(tokenlist, symboldatabase, errorLogger, settings);
         valueFlowForLoop(tokenlist, symboldatabase, errorLogger, settings);
         valueFlowSubFunction(tokenlist, symboldatabase, errorLogger, settings);
@@ -7191,6 +7148,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
         valueFlowUninit(tokenlist, symboldatabase, settings);
         valueFlowUninitPointerAliasDeref(tokenlist);
         if (tokenlist->isCPP()) {
+            valueFlowAfterMove(tokenlist, symboldatabase, settings);
             valueFlowSmartPointer(tokenlist, errorLogger, settings);
             valueFlowIterators(tokenlist, settings);
             valueFlowCondition(IteratorConditionHandler{}, tokenlist, symboldatabase, errorLogger, settings);

test/testnullpointer.cpp

@@ -1171,7 +1171,7 @@ private:
                   "        p = q;\n"
                   "    if (p || *p) { }\n"
                   "}");
-            ASSERT_EQUALS("[test.cpp:5] -> [test.cpp:5]: (warning) Either the condition 'p' is redundant or there is possible null pointer dereference: p.\n", errout.str());
+            ASSERT_EQUALS("[test.cpp:5]: (warning) Possible null pointer dereference: p\n", errout.str());
         }
 
         // ticket #8831 - FP triggered by if/return/else sequence