Skip to content

Commit d80f2fb

Browse files
pfultz2pfultz2
authored
Reapply f1cc3ad and fix performance regression (cppcheck-opensource#3076)
1 parent bb451ca commit d80f2fb

7 files changed

Lines changed: 146 additions & 80 deletions

File tree

‎lib/checkcondition.cpp‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -507,14 +507,12 @@ void CheckCondition::multiCondition()
507507
break;
508508
tok2 = tok2->tokAt(4);
509509

510-
if (tok2->astOperand2() &&
511-
!cond1->hasKnownIntValue() &&
512-
!tok2->astOperand2()->hasKnownIntValue()) {
510+
if (tok2->astOperand2()) {
513511
ErrorPath errorPath;
514512
if (isOverlappingCond(cond1, tok2->astOperand2(), true))
515-
overlappingElseIfConditionError(tok2, cond1->linenr());
513+
overlappingElseIfConditionError(tok2->astOperand2(), cond1->linenr());
516514
else if (isOppositeCond(true, mTokenizer->isCPP(), cond1, tok2->astOperand2(), mSettings->library, true, true, &errorPath))
517-
oppositeElseIfConditionError(cond1, tok2, errorPath);
515+
oppositeElseIfConditionError(cond1, tok2->astOperand2(), errorPath);
518516
}
519517
}
520518
}

‎lib/forwardanalyzer.cpp‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,9 @@ struct ForwardTraversal {
332332
return Progress::Continue;
333333
}
334334

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

@@ -446,7 +448,7 @@ struct ForwardTraversal {
446448
// Traverse then block
447449
thenBranch.escape = isEscapeScope(endBlock, thenBranch.escapeUnknown);
448450
if (thenBranch.check) {
449-
if (updateRange(endCond->next(), endBlock) == Progress::Break)
451+
if (updateRange(endCond->next(), endBlock, depth - 1) == Progress::Break)
450452
return Break();
451453
} else if (!elseBranch.check) {
452454
thenBranch.action = checkScope(endBlock);
@@ -457,7 +459,7 @@ struct ForwardTraversal {
457459
if (hasElse) {
458460
elseBranch.escape = isEscapeScope(endBlock->linkAt(2), elseBranch.escapeUnknown);
459461
if (elseBranch.check) {
460-
Progress result = updateRange(endBlock->tokAt(2), endBlock->linkAt(2));
462+
Progress result = updateRange(endBlock->tokAt(2), endBlock->linkAt(2), depth - 1);
461463
if (result == Progress::Break)
462464
return Break();
463465
} else if (!thenBranch.check) {
@@ -506,7 +508,7 @@ struct ForwardTraversal {
506508
} else if (Token::simpleMatch(tok, "try {")) {
507509
Token* endBlock = tok->next()->link();
508510
Analyzer::Action a = analyzeScope(endBlock);
509-
if (updateRange(tok->next(), endBlock) == Progress::Break)
511+
if (updateRange(tok->next(), endBlock, depth - 1) == Progress::Break)
510512
return Break();
511513
if (a.isModified())
512514
analyzer->lowerToPossible();

‎lib/valueflow.cpp‎

Lines changed: 63 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1427,37 +1427,6 @@ static void valueFlowRightShift(TokenList *tokenList, const Settings* settings)
14271427
}
14281428
}
14291429

1430-
static void valueFlowOppositeCondition(SymbolDatabase *symboldatabase, const Settings *settings)
1431-
{
1432-
for (const Scope &scope : symboldatabase->scopeList) {
1433-
if (scope.type != Scope::eIf)
1434-
continue;
1435-
Token *tok = const_cast<Token *>(scope.classDef);
1436-
if (!Token::simpleMatch(tok, "if ("))
1437-
continue;
1438-
const Token *cond1 = tok->next()->astOperand2();
1439-
if (!cond1 || !cond1->isComparisonOp())
1440-
continue;
1441-
const bool cpp = symboldatabase->isCPP();
1442-
Token *tok2 = tok->linkAt(1);
1443-
while (Token::simpleMatch(tok2, ") {")) {
1444-
tok2 = tok2->linkAt(1);
1445-
if (!Token::simpleMatch(tok2, "} else { if ("))
1446-
break;
1447-
Token *ifOpenBraceTok = tok2->tokAt(4);
1448-
Token *cond2 = ifOpenBraceTok->astOperand2();
1449-
if (!cond2 || !cond2->isComparisonOp())
1450-
continue;
1451-
if (isOppositeCond(true, cpp, cond1, cond2, settings->library, true, true)) {
1452-
ValueFlow::Value value(1);
1453-
value.setKnown();
1454-
setTokenValue(cond2, value, settings);
1455-
}
1456-
tok2 = ifOpenBraceTok->link();
1457-
}
1458-
}
1459-
}
1460-
14611430
static void valueFlowEnumValue(SymbolDatabase * symboldatabase, const Settings * settings)
14621431
{
14631432

@@ -3755,7 +3724,19 @@ static const Token* findIncompleteVar(const Token* start, const Token* end)
37553724
return nullptr;
37563725
}
37573726

3758-
static void valueFlowTerminatingCondition(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings)
3727+
static ValueFlow::Value makeConditionValue(long long val, const Token* condTok, bool assume)
3728+
{
3729+
ValueFlow::Value v(val);
3730+
v.setKnown();
3731+
v.condition = condTok;
3732+
if (assume)
3733+
v.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is true");
3734+
else
3735+
v.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is false");
3736+
return v;
3737+
}
3738+
//
3739+
static void valueFlowConditionExpressions(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings)
37593740
{
37603741
for (const Scope * scope : symboldatabase->functionScopes) {
37613742
if (const Token* incompleteTok = findIncompleteVar(scope->bodyStart, scope->bodyEnd)) {
@@ -3772,28 +3753,60 @@ static void valueFlowTerminatingCondition(TokenList *tokenlist, SymbolDatabase*
37723753
// Skip known values
37733754
if (tok->next()->hasKnownValue())
37743755
continue;
3775-
const Token * parenTok = tok->next();
3756+
Token * parenTok = tok->next();
37763757
if (!Token::simpleMatch(parenTok->link(), ") {"))
37773758
continue;
3778-
const Token * blockTok = parenTok->link()->tokAt(1);
3779-
// Check if the block terminates early
3780-
if (!isEscapeScope(blockTok, tokenlist))
3759+
Token * blockTok = parenTok->link()->tokAt(1);
3760+
const Token* condTok = parenTok->astOperand2();
3761+
if (condTok->hasKnownIntValue())
37813762
continue;
37823763

3783-
const Token* condTok = parenTok->astOperand2();
3784-
ValueFlow::Value v1(0);
3785-
v1.setKnown();
3786-
v1.condition = condTok;
3787-
v1.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is true");
3788-
ExpressionAnalyzer a1(condTok, v1, tokenlist);
3789-
valueFlowGenericForward(blockTok->link()->next(), scope->bodyEnd, a1, settings);
3764+
Token* startTok = blockTok;
3765+
// Inner condition
3766+
{
3767+
std::vector<const Token*> conds = {condTok};
3768+
if (Token::simpleMatch(condTok, "&&")) {
3769+
std::vector<const Token*> args = astFlatten(condTok, "&&");
3770+
conds.insert(conds.end(), args.begin(), args.end());
3771+
}
3772+
for(const Token* condTok2:conds) {
3773+
ExpressionAnalyzer a1(condTok2, makeConditionValue(1, condTok2, true), tokenlist);
3774+
valueFlowGenericForward(startTok, startTok->link(), a1, settings);
3775+
3776+
OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(0, condTok2, true), tokenlist);
3777+
valueFlowGenericForward(startTok, startTok->link(), a2, settings);
3778+
}
3779+
}
3780+
3781+
std::vector<const Token*> conds = {condTok};
3782+
if (Token::simpleMatch(condTok, "||")) {
3783+
std::vector<const Token*> args = astFlatten(condTok, "||");
3784+
conds.insert(conds.end(), args.begin(), args.end());
3785+
}
3786+
3787+
// Check else block
3788+
if (Token::simpleMatch(startTok->link(), "} else {")) {
3789+
startTok = startTok->link()->tokAt(2);
3790+
for(const Token* condTok2:conds) {
3791+
ExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist);
3792+
valueFlowGenericForward(startTok, startTok->link(), a1, settings);
3793+
3794+
OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(1, condTok2, false), tokenlist);
3795+
valueFlowGenericForward(startTok, startTok->link(), a2, settings);
3796+
}
3797+
}
3798+
3799+
// Check if the block terminates early
3800+
if (isEscapeScope(blockTok, tokenlist)) {
3801+
for(const Token* condTok2:conds) {
3802+
ExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist);
3803+
valueFlowGenericForward(startTok->link()->next(), scope->bodyEnd, a1, settings);
3804+
3805+
OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(1, condTok2, false), tokenlist);
3806+
valueFlowGenericForward(startTok->link()->next(), scope->bodyEnd, a2, settings);
3807+
}
3808+
}
37903809

3791-
ValueFlow::Value v2(1);
3792-
v2.setKnown();
3793-
v2.condition = condTok;
3794-
v2.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is false");
3795-
OppositeExpressionAnalyzer a2(true, condTok, v2, tokenlist);
3796-
valueFlowGenericForward(blockTok->link()->next(), scope->bodyEnd, a2, settings);
37973810
}
37983811
}
37993812
}
@@ -6623,6 +6636,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
66236636
valueFlowLifetime(tokenlist, symboldatabase, errorLogger, settings);
66246637
valueFlowBitAnd(tokenlist);
66256638
valueFlowSameExpressions(tokenlist);
6639+
valueFlowConditionExpressions(tokenlist, symboldatabase, errorLogger, settings);
66266640
valueFlowFwdAnalysis(tokenlist, settings);
66276641

66286642
std::size_t values = 0;
@@ -6632,8 +6646,6 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
66326646
valueFlowPointerAliasDeref(tokenlist);
66336647
valueFlowArrayBool(tokenlist);
66346648
valueFlowRightShift(tokenlist, settings);
6635-
valueFlowOppositeCondition(symboldatabase, settings);
6636-
valueFlowTerminatingCondition(tokenlist, symboldatabase, errorLogger, settings);
66376649
valueFlowAfterMove(tokenlist, symboldatabase, errorLogger, settings);
66386650
valueFlowCondition(SimpleConditionHandler{}, tokenlist, symboldatabase, errorLogger, settings);
66396651
valueFlowInferCondition(tokenlist, settings);

‎test/testcondition.cpp‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -533,27 +533,27 @@ class TestCondition : public TestFixture {
533533
" if (a) { b = 1; }\n"
534534
" else { if (a) { b = 2; } }\n"
535535
"}");
536-
ASSERT_EQUALS("[test.cpp:2] -> [test.cpp:3]: (style) Condition 'a' is always false\n", errout.str());
536+
ASSERT_EQUALS("[test.cpp:3]: (style) Expression is always false because 'else if' condition matches previous condition at line 2.\n", errout.str());
537537

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

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

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

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

728728
check("void f(int x) {\n"
729729
" int y = x;\n"

‎test/testsimplifytypedef.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1670,7 +1670,7 @@ class TestSimplifyTypedef : public TestFixture {
16701670
"( ( int ( * * ( * ) ( char * , char * , int , int ) ) ( ) ) global [ 6 ] ) ( \"assoc\" , \"eggdrop\" , 106 , 0 ) ; "
16711671
"}";
16721672
ASSERT_EQUALS(expected, tok(code));
1673-
ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:3]: (debug) valueflow.cpp:1319:valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable global\n", errout.str());
1673+
ASSERT_EQUALS_WITHOUT_LINENUMBERS("[test.cpp:3]: (debug) valueflow.cpp:1319:valueFlowConditionExpressions bailout: Skipping function due to incomplete variable global\n", errout.str());
16741674
}
16751675

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

18831883
void simplifyTypedef83() { // ticket #2620

‎test/testsymboldatabase.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2743,7 +2743,7 @@ class TestSymbolDatabase: public TestFixture {
27432743
check("::y(){x}");
27442744

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

27492749
void symboldatabase20() {

0 commit comments

Comments
 (0)