Extend valueflow comparison ops (#1153)

* Handle else clause when doing a compare

* Break early

* Fix bug in checking no return else

* Escape quotes

* Add equal sign

* Simplify the logic
This commit is contained in:
Paul Fultz II 2018-04-08 02:24:01 -05:00 committed by Daniel Marjamäki
parent b85dda77da
commit aed84abfd5
2 changed files with 77 additions and 39 deletions

View File

@ -2438,20 +2438,25 @@ static void valueFlowAfterCondition(TokenList *tokenlist, SymbolDatabase* symbol
bailout(tokenlist, errorLogger, vartok, "variable is aliased so we just skip all valueflow after condition"); bailout(tokenlist, errorLogger, vartok, "variable is aliased so we just skip all valueflow after condition");
continue; continue;
} }
std::list<ValueFlow::Value> values; std::list<ValueFlow::Value> true_values;
std::list<ValueFlow::Value> false_values;
// TODO: We should add all known values // TODO: We should add all known values
if (numtok) { if (numtok) {
values.push_back(ValueFlow::Value(tok, numtok->values().front().intvalue)); false_values.push_back(ValueFlow::Value(tok, numtok->values().front().intvalue));
true_values.push_back(ValueFlow::Value(tok, numtok->values().front().intvalue));
} else if (lowertok) { } else if (lowertok) {
long long v = lowertok->values().front().intvalue; long long v = lowertok->values().front().intvalue;
values.push_back(ValueFlow::Value(tok, v+1)); true_values.push_back(ValueFlow::Value(tok, v+1));
false_values.push_back(ValueFlow::Value(tok, v));
} else if (uppertok) { } else if (uppertok) {
long long v = uppertok->values().front().intvalue; long long v = uppertok->values().front().intvalue;
values.push_back(ValueFlow::Value(tok, v-1)); true_values.push_back(ValueFlow::Value(tok, v-1));
false_values.push_back(ValueFlow::Value(tok, v));
} else { } else {
values.push_back(ValueFlow::Value(tok, 0LL)); true_values.push_back(ValueFlow::Value(tok, 0LL));
false_values.push_back(ValueFlow::Value(tok, 0LL));
} }
@ -2474,7 +2479,7 @@ static void valueFlowAfterCondition(TokenList *tokenlist, SymbolDatabase* symbol
tokens.push(const_cast<Token*>(rhstok->astOperand1())); tokens.push(const_cast<Token*>(rhstok->astOperand1()));
tokens.push(const_cast<Token*>(rhstok->astOperand2())); tokens.push(const_cast<Token*>(rhstok->astOperand2()));
if (rhstok->varId() == varid) if (rhstok->varId() == varid)
setTokenValue(rhstok, values.front(), settings); setTokenValue(rhstok, true_values.front(), settings);
else if (Token::Match(rhstok, "++|--|=") && Token::Match(rhstok->astOperand1(), "%varid%", varid)) { else if (Token::Match(rhstok, "++|--|=") && Token::Match(rhstok->astOperand1(), "%varid%", varid)) {
assign = true; assign = true;
break; break;
@ -2500,49 +2505,61 @@ static void valueFlowAfterCondition(TokenList *tokenlist, SymbolDatabase* symbol
} }
// start token of conditional code // start token of conditional code
Token *startToken = nullptr; Token *startTokens[] = { nullptr, nullptr };
// based on the comparison, should we check the if or while? // based on the comparison, should we check the if or while?
int codeblock = 0; bool check_if = false;
bool check_else = false;
if (Token::Match(tok, "==|>=|<=|!|>|<")) if (Token::Match(tok, "==|>=|<=|!|>|<"))
codeblock = 1; check_if = true;
else if (Token::Match(tok, "%name%|!=")) if (Token::Match(tok, "%name%|!=|>|<"))
codeblock = 2; check_else = true;
// determine startToken based on codeblock // determine startToken based on codeblock
if (codeblock > 0) { if (check_if || check_else) {
// if astParent is "!" we need to invert codeblock // if astParent is "!" we need to invert codeblock
const Token *parent = tok->astParent(); const Token *parent = tok->astParent();
while (parent && parent->str() == "&&") while (parent && parent->str() == "&&")
parent = parent->astParent(); parent = parent->astParent();
if (parent && parent->str() == "!") if (parent && parent->str() == "!") {
codeblock = (codeblock == 1) ? 2 : 1; check_if = !check_if;
check_else = !check_else;
}
// convert codeblock to a startToken // convert codeblock to a startToken
if (codeblock == 1 && Token::simpleMatch(top->link(), ") {")) if (check_if && Token::simpleMatch(top->link(), ") {"))
startToken = top->link()->next(); startTokens[0] = top->link()->next();
else if (Token::simpleMatch(top->link()->linkAt(1), "} else {")) if (check_else && Token::simpleMatch(top->link()->linkAt(1), "} else {"))
startToken = top->link()->linkAt(1)->tokAt(2); startTokens[1] = top->link()->linkAt(1)->tokAt(2);
} }
if (startToken) { bool bail = false;
if (values.size() == 1U && Token::Match(tok, "==|!")) {
const Token *parent = tok->astParent(); for(int i=0;i<2;i++) {
while (parent && parent->str() == "&&") Token * startToken = startTokens[i];
parent = parent->astParent(); if(startToken) {
if (parent && parent->str() == "(") std::list<ValueFlow::Value> & values = (i==0 ? true_values : false_values);
values.front().setKnown(); if (values.size() == 1U && Token::Match(tok, "==|!")) {
} const Token *parent = tok->astParent();
if (!valueFlowForward(startToken->next(), startToken->link(), var, varid, values, true, false, tokenlist, errorLogger, settings)) while (parent && parent->str() == "&&")
continue; parent = parent->astParent();
values.front().setPossible(); if (parent && parent->str() == "(")
if (isVariableChanged(startToken, startToken->link(), varid, var->isGlobal(), settings)) { values.front().setKnown();
// TODO: The endToken should not be startToken->link() in the valueFlowForward call }
if (settings->debugwarnings)
bailout(tokenlist, errorLogger, startToken->link(), "valueFlowAfterCondition: " + var->name() + " is changed in conditional block"); valueFlowForward(startTokens[i]->next(), startTokens[i]->link(), var, varid, values, true, false, tokenlist, errorLogger, settings);
continue; values.front().setPossible();
if (isVariableChanged(startTokens[i], startTokens[i]->link(), varid, var->isGlobal(), settings)) {
// TODO: The endToken should not be startTokens[i]->link() in the valueFlowForward call
if (settings->debugwarnings)
bailout(tokenlist, errorLogger, startTokens[i]->link(), "valueFlowAfterCondition: " + var->name() + " is changed in conditional block");
bail = true;
break;
}
} }
} }
if(bail)
continue;
// After conditional code.. // After conditional code..
if (Token::simpleMatch(top->link(), ") {")) { if (Token::simpleMatch(top->link(), ") {")) {
@ -2554,7 +2571,9 @@ static void valueFlowAfterCondition(TokenList *tokenlist, SymbolDatabase* symbol
continue; continue;
} }
bool isreturn = (codeblock == 1 && isReturnScope(after)); std::list<ValueFlow::Value> * values = nullptr;
if (check_else || !isReturnScope(after))
values = &false_values;
if (Token::simpleMatch(after, "} else {")) { if (Token::simpleMatch(after, "} else {")) {
after = after->linkAt(2); after = after->linkAt(2);
@ -2563,14 +2582,15 @@ static void valueFlowAfterCondition(TokenList *tokenlist, SymbolDatabase* symbol
bailout(tokenlist, errorLogger, after, "possible noreturn scope"); bailout(tokenlist, errorLogger, after, "possible noreturn scope");
continue; continue;
} }
isreturn |= (codeblock == 2 && isReturnScope(after)); if (check_if || !isReturnScope(after))
values = &true_values;
} }
if (!isreturn) { if (values) {
// TODO: constValue could be true if there are no assignments in the conditional blocks and // TODO: constValue could be true if there are no assignments in the conditional blocks and
// perhaps if there are no && and no || in the condition // perhaps if there are no && and no || in the condition
bool constValue = false; bool constValue = false;
valueFlowForward(after->next(), top->scope()->classEnd, var, varid, values, constValue, false, tokenlist, errorLogger, settings); valueFlowForward(after->next(), top->scope()->classEnd, var, varid, *values, constValue, false, tokenlist, errorLogger, settings);
} }
} }
} }

View File

@ -1716,7 +1716,6 @@ private:
void valueFlowAfterCondition() { void valueFlowAfterCondition() {
const char *code; const char *code;
// in if // in if
code = "void f(int x) {\n" code = "void f(int x) {\n"
" if (x == 123) {\n" " if (x == 123) {\n"
@ -1808,6 +1807,25 @@ private:
"}"; "}";
ASSERT_EQUALS(false, testValueOfX(code, 3U, 0)); ASSERT_EQUALS(false, testValueOfX(code, 3U, 0));
code = "void f(int x) {\n"
" if (x != 123) { throw ""; }\n"
" a = x;\n"
"}";
ASSERT_EQUALS(true, testValueOfX(code, 3U, 123));
code = "void f(int x) {\n"
" if (x < 123) { }\n"
" else { a = x; }\n"
"}";
ASSERT_EQUALS(true, testValueOfX(code, 3U, 123));
code = "void f(int x) {\n"
" if (x < 123) { throw \"\"; }\n"
" a = x;\n"
"}";
ASSERT_EQUALS(true, testValueOfX(code, 3U, 123));
// if (var) // if (var)
code = "void f(int x) {\n" code = "void f(int x) {\n"
" if (x) { a = x; }\n" // <- x is not 0 " if (x) { a = x; }\n" // <- x is not 0