Skip to content

Commit f1cc3ad

Browse files
pfultz2pfultz2
authored
Refactor valueFlowTerminatingCondition to handle inner conditions and complex conditions (cppcheck-opensource#3060)
1 parent d05acf3 commit f1cc3ad

6 files changed

Lines changed: 138 additions & 76 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/valueflow.cpp‎

Lines changed: 61 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

@@ -3756,7 +3725,19 @@ static const Token* findIncompleteVar(const Token* start, const Token* end)
37563725
return nullptr;
37573726
}
37583727

3759-
static void valueFlowTerminatingCondition(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings)
3728+
ValueFlow::Value makeConditionValue(long long val, const Token* condTok, bool assume)
3729+
{
3730+
ValueFlow::Value v(val);
3731+
v.setKnown();
3732+
v.condition = condTok;
3733+
if (assume)
3734+
v.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is true");
3735+
else
3736+
v.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is false");
3737+
return v;
3738+
}
3739+
//
3740+
static void valueFlowConditionExpressions(TokenList *tokenlist, SymbolDatabase* symboldatabase, ErrorLogger *errorLogger, const Settings *settings)
37603741
{
37613742
for (const Scope * scope : symboldatabase->functionScopes) {
37623743
if (const Token* incompleteTok = findIncompleteVar(scope->bodyStart, scope->bodyEnd)) {
@@ -3773,28 +3754,58 @@ static void valueFlowTerminatingCondition(TokenList *tokenlist, SymbolDatabase*
37733754
// Skip known values
37743755
if (tok->next()->hasKnownValue())
37753756
continue;
3776-
const Token * parenTok = tok->next();
3757+
Token * parenTok = tok->next();
37773758
if (!Token::simpleMatch(parenTok->link(), ") {"))
37783759
continue;
3779-
const Token * blockTok = parenTok->link()->tokAt(1);
3760+
Token * blockTok = parenTok->link()->tokAt(1);
3761+
const Token* condTok = parenTok->astOperand2();
3762+
3763+
Token* startTok = blockTok;
3764+
// Inner condition
3765+
{
3766+
std::vector<const Token*> conds = {condTok};
3767+
if (Token::simpleMatch(condTok, "&&")) {
3768+
std::vector<const Token*> args = astFlatten(condTok, "&&");
3769+
conds.insert(conds.end(), args.begin(), args.end());
3770+
}
3771+
for(const Token* condTok2:conds) {
3772+
ExpressionAnalyzer a1(condTok2, makeConditionValue(1, condTok2, true), tokenlist);
3773+
valueFlowGenericForward(startTok, startTok->link(), a1, settings);
3774+
3775+
OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(0, condTok2, true), tokenlist);
3776+
valueFlowGenericForward(startTok, startTok->link(), a2, settings);
3777+
}
3778+
}
3779+
3780+
std::vector<const Token*> conds = {condTok};
3781+
if (Token::simpleMatch(condTok, "||")) {
3782+
std::vector<const Token*> args = astFlatten(condTok, "||");
3783+
conds.insert(conds.end(), args.begin(), args.end());
3784+
}
3785+
3786+
// Check else block
3787+
if (Token::simpleMatch(startTok->link(), "} else {")) {
3788+
startTok = startTok->link()->tokAt(2);
3789+
for(const Token* condTok2:conds) {
3790+
ExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist);
3791+
valueFlowGenericForward(startTok, startTok->link(), a1, settings);
3792+
3793+
OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(1, condTok2, false), tokenlist);
3794+
valueFlowGenericForward(startTok, startTok->link(), a2, settings);
3795+
}
3796+
}
3797+
37803798
// Check if the block terminates early
3781-
if (!isEscapeScope(blockTok, tokenlist))
3782-
continue;
3799+
if (isEscapeScope(blockTok, tokenlist)) {
3800+
for(const Token* condTok2:conds) {
3801+
ExpressionAnalyzer a1(condTok2, makeConditionValue(0, condTok2, false), tokenlist);
3802+
valueFlowGenericForward(startTok->link()->next(), scope->bodyEnd, a1, settings);
37833803

3784-
const Token* condTok = parenTok->astOperand2();
3785-
ValueFlow::Value v1(0);
3786-
v1.setKnown();
3787-
v1.condition = condTok;
3788-
v1.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is true");
3789-
ExpressionAnalyzer a1(condTok, v1, tokenlist);
3790-
valueFlowGenericForward(blockTok->link()->next(), scope->bodyEnd, a1, settings);
3804+
OppositeExpressionAnalyzer a2(true, condTok2, makeConditionValue(1, condTok2, false), tokenlist);
3805+
valueFlowGenericForward(startTok->link()->next(), scope->bodyEnd, a2, settings);
3806+
}
3807+
}
37913808

3792-
ValueFlow::Value v2(1);
3793-
v2.setKnown();
3794-
v2.condition = condTok;
3795-
v2.errorPath.emplace_back(condTok, "Assuming condition '" + condTok->expressionString() + "' is false");
3796-
OppositeExpressionAnalyzer a2(true, condTok, v2, tokenlist);
3797-
valueFlowGenericForward(blockTok->link()->next(), scope->bodyEnd, a2, settings);
37983809
}
37993810
}
38003811
}
@@ -6624,6 +6635,7 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
66246635
valueFlowLifetime(tokenlist, symboldatabase, errorLogger, settings);
66256636
valueFlowBitAnd(tokenlist);
66266637
valueFlowSameExpressions(tokenlist);
6638+
valueFlowConditionExpressions(tokenlist, symboldatabase, errorLogger, settings);
66276639
valueFlowFwdAnalysis(tokenlist, settings);
66286640

66296641
std::size_t values = 0;
@@ -6633,8 +6645,6 @@ void ValueFlow::setValues(TokenList *tokenlist, SymbolDatabase* symboldatabase,
66336645
valueFlowPointerAliasDeref(tokenlist);
66346646
valueFlowArrayBool(tokenlist);
66356647
valueFlowRightShift(tokenlist, settings);
6636-
valueFlowOppositeCondition(symboldatabase, settings);
6637-
valueFlowTerminatingCondition(tokenlist, symboldatabase, errorLogger, settings);
66386648
valueFlowAfterMove(tokenlist, symboldatabase, errorLogger, settings);
66396649
valueFlowCondition(SimpleConditionHandler{}, tokenlist, symboldatabase, errorLogger, settings);
66406650
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() {

‎test/testvalueflow.cpp‎

Lines changed: 64 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ class TestValueFlow : public TestFixture {
122122

123123
TEST_CASE(valueFlowUninit);
124124

125-
TEST_CASE(valueFlowTerminatingCond);
125+
TEST_CASE(valueFlowConditionExpressions);
126126

127127
TEST_CASE(valueFlowContainerSize);
128128

@@ -1266,7 +1266,7 @@ class TestValueFlow : public TestFixture {
12661266
" if (x == 123) {}\n"
12671267
"}");
12681268
ASSERT_EQUALS_WITHOUT_LINENUMBERS(
1269-
"[test.cpp:2]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable y\n",
1269+
"[test.cpp:2]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable y\n",
12701270
errout.str());
12711271
}
12721272

@@ -1383,7 +1383,7 @@ class TestValueFlow : public TestFixture {
13831383
" y = ((x<0) ? x : ((x==2)?3:4));\n"
13841384
"}");
13851385
ASSERT_EQUALS_WITHOUT_LINENUMBERS(
1386-
"[test.cpp:2]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable y\n",
1386+
"[test.cpp:2]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable y\n",
13871387
errout.str());
13881388

13891389
bailout("int f(int x) {\n"
@@ -1448,7 +1448,7 @@ class TestValueFlow : public TestFixture {
14481448
" if (x == 123) {}\n"
14491449
"}");
14501450
ASSERT_EQUALS_WITHOUT_LINENUMBERS(
1451-
"[test.cpp:2]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable b\n",
1451+
"[test.cpp:2]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable b\n",
14521452
errout.str());
14531453

14541454
code = "void f(int x, bool abc) {\n"
@@ -1497,7 +1497,7 @@ class TestValueFlow : public TestFixture {
14971497
" };\n"
14981498
"}");
14991499
ASSERT_EQUALS_WITHOUT_LINENUMBERS(
1500-
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable a\n",
1500+
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n",
15011501
errout.str());
15021502

15031503
bailout("void f(int x, int y) {\n"
@@ -1507,7 +1507,7 @@ class TestValueFlow : public TestFixture {
15071507
" };\n"
15081508
"}");
15091509
ASSERT_EQUALS_WITHOUT_LINENUMBERS(
1510-
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable a\n",
1510+
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n",
15111511
errout.str());
15121512
}
15131513

@@ -1519,7 +1519,7 @@ class TestValueFlow : public TestFixture {
15191519
" M;\n"
15201520
"}");
15211521
ASSERT_EQUALS_WITHOUT_LINENUMBERS(
1522-
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable a\n"
1522+
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n"
15231523
"[test.cpp:4]: (debug) valueflow.cpp:1260:(valueFlow) bailout: variable 'x', condition is defined in macro\n",
15241524
errout.str());
15251525

@@ -1529,7 +1529,7 @@ class TestValueFlow : public TestFixture {
15291529
" FREE(x);\n"
15301530
"}");
15311531
ASSERT_EQUALS_WITHOUT_LINENUMBERS(
1532-
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable a\n"
1532+
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n"
15331533
"[test.cpp:4]: (debug) valueflow.cpp:1260:(valueFlow) bailout: variable 'x', condition is defined in macro\n",
15341534
errout.str());
15351535
}
@@ -1543,7 +1543,7 @@ class TestValueFlow : public TestFixture {
15431543
" if (x==123){}\n"
15441544
"}");
15451545
ASSERT_EQUALS_WITHOUT_LINENUMBERS(
1546-
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowTerminatingCondition bailout: Skipping function due to incomplete variable a\n",
1546+
"[test.cpp:3]: (debug) valueflow.cpp::valueFlowConditionExpressions bailout: Skipping function due to incomplete variable a\n",
15471547
errout.str());
15481548

15491549
// #5721 - FP
@@ -4202,7 +4202,7 @@ class TestValueFlow : public TestFixture {
42024202
ASSERT_EQUALS(0, values.size());
42034203
}
42044204

4205-
void valueFlowTerminatingCond() {
4205+
void valueFlowConditionExpressions() {
42064206
const char* code;
42074207

42084208
// opposite condition
@@ -4272,6 +4272,60 @@ class TestValueFlow : public TestFixture {
42724272
"}\n";
42734273
ASSERT_EQUALS(false, valueOfTok(code, "!=").intvalue == 1);
42744274

4275+
code = "void f(bool b, int i, int j) {\n"
4276+
" if (b || i == j) return;\n"
4277+
" if(i != j) {}\n"
4278+
"}\n";
4279+
ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 1);
4280+
4281+
code = "void f(bool b, int i, int j) {\n"
4282+
" if (b && i == j) return;\n"
4283+
" if(i != j) {}\n"
4284+
"}\n";
4285+
ASSERT_EQUALS(true, tokenValues(code, "!=").empty());
4286+
4287+
code = "void f(int i, int j) {\n"
4288+
" if (i == j) {\n"
4289+
" if (i != j) {}\n"
4290+
" }\n"
4291+
"}\n";
4292+
ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 0);
4293+
4294+
code = "void f(int i, int j) {\n"
4295+
" if (i == j) {} else {\n"
4296+
" if (i != j) {}\n"
4297+
" }\n"
4298+
"}\n";
4299+
ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 1);
4300+
4301+
code = "void f(bool b, int i, int j) {\n"
4302+
" if (b && i == j) {\n"
4303+
" if (i != j) {}\n"
4304+
" }\n"
4305+
"}\n";
4306+
ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 0);
4307+
4308+
code = "void f(bool b, int i, int j) {\n"
4309+
" if (b || i == j) {\n"
4310+
" if (i != j) {}\n"
4311+
" }\n"
4312+
"}\n";
4313+
ASSERT_EQUALS(true, tokenValues(code, "!=").empty());
4314+
4315+
code = "void f(bool b, int i, int j) {\n"
4316+
" if (b || i == j) {} else {\n"
4317+
" if (i != j) {}\n"
4318+
" }\n"
4319+
"}\n";
4320+
ASSERT_EQUALS(true, valueOfTok(code, "!=").intvalue == 1);
4321+
4322+
code = "void f(bool b, int i, int j) {\n"
4323+
" if (b && i == j) {} else {\n"
4324+
" if (i != j) {}\n"
4325+
" }\n"
4326+
"}\n";
4327+
ASSERT_EQUALS(true, tokenValues(code, "!=").empty());
4328+
42754329
code = "void foo()\n" // #8924
42764330
"{\n"
42774331
" if ( this->FileIndex >= 0 )\n"

0 commit comments

Comments
 (0)