From 69eaa9dfd9e648b1eeaf8badc8bd4fa469d208f1 Mon Sep 17 00:00:00 2001 From: Paul Fultz II Date: Wed, 11 Aug 2021 01:37:37 -0500 Subject: [PATCH] Refactor: ConditionHandler cleanup (#3394) --- lib/valueflow.cpp | 76 +++++++++------------------------------- test/testnullpointer.cpp | 2 +- 2 files changed, 18 insertions(+), 60 deletions(-) diff --git a/lib/valueflow.cpp b/lib/valueflow.cpp index 808120562..eff3ff227 100644 --- a/lib/valueflow.cpp +++ b/lib/valueflow.cpp @@ -4526,29 +4526,22 @@ struct ConditionHandler { virtual std::vector parse(const Token* tok, const Settings* settings) const = 0; - void traverseCondition( - TokenList* tokenlist, - SymbolDatabase* symboldatabase, - ErrorLogger* errorLogger, - const Settings* settings, - const std::function< - void(const Condition& cond, Token* tok, const Scope* scope, const std::vector& vars)>& f) const { + void traverseCondition(TokenList* tokenlist, + SymbolDatabase* symboldatabase, + const std::function& f) const + { for (const Scope *scope : symboldatabase->functionScopes) { - std::set aliased; for (Token *tok = const_cast(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, settings)) { + for (const Condition& cond : parse(tok, tokenlist->getSettings())) { if (!cond.vartok) continue; if (cond.vartok->exprId() == 0) @@ -4557,27 +4550,10 @@ 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 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, vars); + f(cond, tok, scope); } - // *INDENT-ON* } } } @@ -4586,12 +4562,7 @@ struct ConditionHandler { SymbolDatabase* symboldatabase, ErrorLogger* errorLogger, const Settings* settings) const { - traverseCondition( - tokenlist, - symboldatabase, - errorLogger, - settings, - [&](const Condition& cond, Token* tok, const Scope*, const std::vector&) { + traverseCondition(tokenlist, symboldatabase, [&](const Condition& cond, Token* tok, const Scope*) { if (cond.vartok->exprId() == 0) return; @@ -4699,12 +4670,7 @@ struct ConditionHandler { SymbolDatabase* symboldatabase, ErrorLogger* errorLogger, const Settings* settings) const { - traverseCondition( - tokenlist, - symboldatabase, - errorLogger, - settings, - [&](const Condition& cond, Token* tok, const Scope* scope, const std::vector& vars) { + traverseCondition(tokenlist, symboldatabase, [&](const Condition& cond, Token* tok, const Scope* scope) { 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(); }); - for(Token* start:nextExprs) { + return v.isIntValue() || v.isFloatValue(); + }))) + 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); diff --git a/test/testnullpointer.cpp b/test/testnullpointer.cpp index a00958d51..3a33c1000 100644 --- a/test/testnullpointer.cpp +++ b/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