Skip to content

Reapply f1cc3ad and fix performance regression #3076

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
danmar merged 3 commits into from
Jan 23, 2021
Merged

Reapply f1cc3ad and fix performance regression #3076

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8f3c0bd
Re apply commit
pfultz2 Jan 22, 2021
04d3aa8
Limit recursion depth
pfultz2 Jan 22, 2021
2c35b51
Remove extra header
pfultz2 Jan 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions lib/checkcondition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,14 +507,12 @@ void CheckCondition::multiCondition()
break;
tok2 = tok2->tokAt(4);

if (tok2->astOperand2() &&
!cond1->hasKnownIntValue() &&
!tok2->astOperand2()->hasKnownIntValue()) {
if (tok2->astOperand2()) {
ErrorPath errorPath;
if (isOverlappingCond(cond1, tok2->astOperand2(), true))
overlappingElseIfConditionError(tok2, cond1->linenr());
overlappingElseIfConditionError(tok2->astOperand2(), cond1->linenr());
else if (isOppositeCond(true, mTokenizer->isCPP(), cond1, tok2->astOperand2(), mSettings->library, true, true, &errorPath))
oppositeElseIfConditionError(cond1, tok2, errorPath);
oppositeElseIfConditionError(cond1, tok2->astOperand2(), errorPath);
}
}
}
Expand Down
10 changes: 6 additions & 4 deletions lib/forwardanalyzer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,9 @@ struct ForwardTraversal {
return Progress::Continue;
}

Progress updateRange(Token* start, const Token* end) {
Progress updateRange(Token* start, const Token* end, int depth = 20) {
if (depth < 0)
return Break(Terminate::Bail);
for (Token* tok = start; tok && tok != end; tok = tok->next()) {
Token* next = nullptr;

Expand Down Expand Up @@ -450,7 +452,7 @@ struct ForwardTraversal {
// Traverse then block
thenBranch.escape = isEscapeScope(endBlock, thenBranch.escapeUnknown);
if (thenBranch.check) {
if (updateRange(endCond->next(), endBlock) == Progress::Break)
if (updateRange(endCond->next(), endBlock, depth - 1) == Progress::Break)
return Break();
} else if (!elseBranch.check) {
thenBranch.action = checkScope(endBlock);
Expand All @@ -461,7 +463,7 @@ struct ForwardTraversal {
if (hasElse) {
elseBranch.escape = isEscapeScope(endBlock->linkAt(2), elseBranch.escapeUnknown);
if (elseBranch.check) {
Progress result = updateRange(endBlock->tokAt(2), endBlock->linkAt(2));
Progress result = updateRange(endBlock->tokAt(2), endBlock->linkAt(2), depth - 1);
if (result == Progress::Break)
return Break();
} else if (!thenBranch.check) {
Expand Down Expand Up @@ -510,7 +512,7 @@ struct ForwardTraversal {
} else if (Token::simpleMatch(tok, "try {")) {
Token* endBlock = tok->next()->link();
Analyzer::Action a = analyzeScope(endBlock);
if (updateRange(tok->next(), endBlock) == Progress::Break)
if (updateRange(tok->next(), endBlock, depth - 1) == Progress::Break)
return Break();
if (a.isModified())
analyzer->lowerToPossible();
Expand Down
114 changes: 63 additions & 51 deletions lib/valueflow.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1427,37 +1427,6 @@ static void valueFlowRightShift(TokenList *tokenList, const Settings* settings)
}
}

static void valueFlowOppositeCondition(SymbolDatabase *symboldatabase, const Settings *settings)
{
for (const Scope &scope : symboldatabase->scopeList) {
if (scope.type != Scope::eIf)
continue;
Token *tok = const_cast<Token *>(scope.classDef);
if (!Token::simpleMatch(tok, "if ("))
continue;
const Token *cond1 = tok->next()->astOperand2();
if (!cond1 || !cond1->isComparisonOp())
continue;
const bool cpp = symboldatabase->isCPP();
Token *tok2 = tok->linkAt(1);
while (Token::simpleMatch(tok2, ") {")) {
tok2 = tok2->linkAt(1);
if (!Token::simpleMatch(tok2, "} else { if ("))
break;
Token *ifOpenBraceTok = tok2->tokAt(4);
Token *cond2 = ifOpenBraceTok->astOperand2();
if (!cond2 || !cond2->isComparisonOp())
continue;
if (isOppositeCond(true, cpp, cond1, cond2, settings->library, true, true)) {
ValueFlow::Value value(1);
value.setKnown();
setTokenValue(cond2, value, settings);
}
tok2 = ifOpenBraceTok->link();
}
}
}

static void valueFlowEnumValue(SymbolDatabase * symboldatabase, const Settings * settings)
{

Expand Down Expand Up @@ -3756,7 +3725,19 @@ static const Token* findIncompleteVar(const Token* start, const Token* end)
return nullptr;
}

static void valueFlowTerminatingCondition(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings)
static ValueFlow::Value makeConditionValue(long long val, const Token* condTok, bool assume)
{
ValueFlow::Value v(val);
v.setKnown();
v.condition = condTok;
if (assume)
v.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is true");
else
v.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is false");
return v;
}
//
static void valueFlowConditionExpressions(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings)
{
for (const Scope * scope : symboldatabase->functionScopes) {
if (const Token* incompleteTok = findIncompleteVar(scope->bodyStart, scope->bodyEnd)) {
Expand All @@ -3773,28 +3754,60 @@ static void valueFlowTerminatingCondition(TokenList *tokenlist, SymbolDatabase*
// Skip known values
if (tok->next()->hasKnownValue())
continue;
const Token * parenTok = tok->next();
Token * parenTok = tok->next();
if (!Token::simpleMatch(parenTok->link(), ") {"))
continue;
const Token * blockTok = parenTok->link()->tokAt(1);
// Check if the block terminates early
if (!isEscapeScope(blockTok, tokenlist))
Token * blockTok = parenTok->link()->tokAt(1);
const Token* condTok = parenTok->astOperand2();
if (condTok->hasKnownIntValue())
continue;

const Token* condTok = parenTok->astOperand2();
ValueFlow::Value v1(0);
v1.setKnown();
v1.condition = condTok;
v1.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is true");
ExpressionAnalyzer a1(condTok, v1, tokenlist);
valueFlowGenericForward(blockTok->link()->next(), scope->bodyEnd, a1, settings);
Token* startTok = blockTok;
// Inner condition
{
std::vector<const Token*> conds = {condTok};
if (Token::simpleMatch(condTok, "&&")) {
std::vector<const Token*> args = astFlatten(condTok, "&&");
conds.insert(conds.end(), args.begin(), args.end());
}
for(const Token* condTok2:conds) {
ExpressionAnalyzer a1(condTok2, makeConditionValue(1, condTok2, true), tokenlist);
valueFlowGenericForward(startTok, startTok->link(), a1, settings);

OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(0, condTok2, true), tokenlist);
valueFlowGenericForward(startTok, startTok->link(), a2, settings);
}
}

std::vector<const Token*> conds = {condTok};
if (Token::simpleMatch(condTok, "||")) {
std::vector<const Token*> args = astFlatten(condTok, "||");
conds.insert(conds.end(), args.begin(), args.end());
}

// Check else block
if (Token::simpleMatch(startTok->link(), "} else {")) {
startTok = startTok->link()->tokAt(2);
for(const Token* condTok2:conds) {
ExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist);
valueFlowGenericForward(startTok, startTok->link(), a1, settings);

OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(1, condTok2, false), tokenlist);
valueFlowGenericForward(startTok, startTok->link(), a2, settings);
}
}

// Check if the block terminates early
if (isEscapeScope(blockTok, tokenlist)) {
for(const Token* condTok2:conds) {
ExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist);
valueFlowGenericForward(startTok->link()->next(), scope->bodyEnd, a1, settings);

OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(1, condTok2, false), tokenlist);
valueFlowGenericForward(startTok->link()->next(), scope->bodyEnd, a2, settings);
}
}

ValueFlow::Value v2(1);
v2.setKnown();
v2.condition = condTok;
v2.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is false");
OppositeExpressionAnalyzer a2(true, condTok, v2, tokenlist);
valueFlowGenericForward(blockTok->link()->next(), scope->bodyEnd, a2, settings);
}
}
}
Expand Down Expand Up @@ -6624,6 +6637,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
valueFlowLifetime(tokenlist, symboldatabase, errorLogger, settings);
valueFlowBitAnd(tokenlist);
valueFlowSameExpressions(tokenlist);
valueFlowConditionExpressions(tokenlist, symboldatabase, errorLogger, settings);
valueFlowFwdAnalysis(tokenlist, settings);

std::size_t values = 0;
Expand All @@ -6633,8 +6647,6 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
valueFlowPointerAliasDeref(tokenlist);
valueFlowArrayBool(tokenlist);
valueFlowRightShift(tokenlist, settings);
valueFlowOppositeCondition(symboldatabase, settings);
valueFlowTerminatingCondition(tokenlist, symboldatabase, errorLogger, settings);
valueFlowAfterMove(tokenlist, symboldatabase, errorLogger, settings);
valueFlowCondition(SimpleConditionHandler{}, tokenlist, symboldatabase, errorLogger, settings);
valueFlowInferCondition(tokenlist, settings);
Expand Down
14 changes: 7 additions & 7 deletions test/testcondition.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,27 +533,27 @@ class TestCondition : public TestFixture {
" if (a) { b = 1; }\n"
" else { if (a) { b = 2; } }\n"
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Condition 'a' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str());

check("void f(int a, int &b) {\n"
" if (a) { b = 1; }\n"
" else { if (a) { b = 2; } }\n"
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Condition 'a' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str());

check("void f(int a, int &b) {\n"
" if (a == 1) { b = 1; }\n"
" else { if (a == 2) { b = 2; }\n"
" else { if (a == 1) { b = 3; } } }\n"
"}");
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:4]: (style) Condition 'a==1' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str());

check("void f(int a, int &b) {\n"
" if (a == 1) { b = 1; }\n"
" else { if (a == 2) { b = 2; }\n"
" else { if (a == 2) { b = 3; } } }\n"
"}");
ASSERT_EQUALS("[test.cpp:3] -> [test.cpp:4]: (style) Condition 'a==2' is always false\n", errout.str());
ASSERT_EQUALS("[test.cpp:4]: (style) Expression is always false because 'else if' condition matches previous condition at line 3.\n", errout.str());

check("void f(int a, int &b) {\n"
" if (a++) { b = 1; }\n"
Expand Down Expand Up @@ -721,9 +721,9 @@ class TestCondition : public TestFixture {
" if (x) {}\n"
" else if (!x) {}\n"
"}");
ASSERT_EQUALS("test.cpp:3:style:Condition '!x' is always true\n"
"test.cpp:2:note:condition 'x'\n"
"test.cpp:3:note:Condition '!x' is always true\n", errout.str());
ASSERT_EQUALS("test.cpp:3:style:Expression is always true because 'else if' condition is opposite to previous condition at line 2.\n"
"test.cpp:2:note:first condition\n"
"test.cpp:3:note:else if condition is opposite to first condition\n", errout.str());

check("void f(int x) {\n"
" int y = x;\n"
Expand Down
4 changes: 2 additions & 2 deletions test/testsimplifytypedef.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1670,7 +1670,7 @@ class TestSimplifyTypedef : public TestFixture {
"( ( int ( * * ( * ) ( char * , char * , int , int ) ) ( ) ) global [ 6 ] ) ( \"assoc\" , \"eggdrop\" , 106 , 0 ) ; "
"}";
ASSERT_EQUALS(expected, tok(code));
ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:3]: (debug) valueflow.cpp:1319:valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable global\n", errout.str());
ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:3]: (debug) valueflow.cpp:1319:valueFlowConditionExpressions bailout: Skipping function due to incomplete variable global\n", errout.str());
}

void simplifyTypedef68() { // ticket #2355
Expand Down Expand Up @@ -1877,7 +1877,7 @@ class TestSimplifyTypedef : public TestFixture {
" B * b = new B;\n"
" b->f = new A::F * [ 10 ];\n"
"}");
ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:12]: (debug) valueflow.cpp:1319:valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable idx\n", errout.str());
ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:12]: (debug) valueflow.cpp:1319:valueFlowConditionExpressions bailout: Skipping function due to incomplete variable idx\n", errout.str());
}

void simplifyTypedef83() { // ticket #2620
Expand Down
2 changes: 1 addition & 1 deletion test/testsymboldatabase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2743,7 +2743,7 @@ class TestSymbolDatabase: public TestFixture {
check("::y(){x}");

ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:1]: (debug) Executable scope 'y' with unknown function.\n"
"[test.cpp:1]: (debug) valueflow.cpp:1321:valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable x\n", errout.str());
"[test.cpp:1]: (debug) valueflow.cpp:1321:valueFlowConditionExpressions bailout: Skipping function due to incomplete variable x\n", errout.str());
}

void symboldatabase20() {
Expand Down
Loading