Skip to content

Commit 5818866

Browse files
danmardanmar
committed
CheckBool::checkComparisonOfBoolExpressionWithInt: Rewrite the check using AST instead of token list
1 parent 9b307cf commit 5818866

2 files changed

Lines changed: 36 additions & 118 deletions

File tree

‎lib/checkbool.cpp‎

Lines changed: 33 additions & 115 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ namespace {
3131

3232
static bool astIsBool(const Token *expr)
3333
{
34-
return Token::Match(expr, "%comp%|%bool%|%oror%|&&");
34+
return Token::Match(expr, "%comp%|%bool%|%oror%|&&|!");
3535
}
3636

3737
//---------------------------------------------------------------------------
@@ -357,37 +357,6 @@ void CheckBool::assignBoolToPointerError(const Token *tok)
357357
"Boolean value assigned to pointer.");
358358
}
359359

360-
/**
361-
* @brief Is the result of the LHS expression non-bool?
362-
* @param tok last token in lhs
363-
* @return true => lhs result is non-bool. false => lhs result type is unknown or bool
364-
*/
365-
static bool isNonBoolLHSExpr(const Token *tok)
366-
{
367-
// return value. only return true if we "know" it's a non-bool expression
368-
bool nonBoolExpr = false;
369-
370-
for (; tok; tok = tok->previous()) {
371-
if (tok->str() == ")") {
372-
if (!Token::Match(tok->link()->previous(), "&&|%oror%|( ("))
373-
tok = tok->link();
374-
} else if (tok->str() == "(" || tok->str() == "[")
375-
break;
376-
else if (tok->isNumber())
377-
nonBoolExpr = true;
378-
else if (tok->isArithmeticalOp()) {
379-
return true;
380-
} else if (tok->isComparisonOp() || (tok->str() == "!" && tok->previous()->str()=="("))
381-
return false;
382-
else if (Token::Match(tok,"[;{}=?:&|^,]"))
383-
break;
384-
else if (Token::Match(tok, "&&|%oror%|and|or"))
385-
break;
386-
}
387-
388-
return nonBoolExpr;
389-
}
390-
391360
//-----------------------------------------------------------------------------
392361
//-----------------------------------------------------------------------------
393362
void CheckBool::checkComparisonOfBoolExpressionWithInt()
@@ -401,101 +370,50 @@ void CheckBool::checkComparisonOfBoolExpressionWithInt()
401370
for (std::size_t i = 0; i < functions; ++i) {
402371
const Scope * scope = symbolDatabase->functionScopes[i];
403372
for (const Token* tok = scope->classStart->next(); tok != scope->classEnd; tok = tok->next()) {
373+
if (!tok->isComparisonOp())
374+
continue;
375+
404376
// Skip template parameters
405377
if (tok->str() == "<" && tok->link()) {
406378
tok = tok->link();
407379
continue;
408380
}
409381

410382
const Token* numTok = 0;
411-
const Token* opTok = 0;
412-
char op = 0;
413-
if (Token::Match(tok, "&&|%oror% %any% ) %comp% %any%")) {
414-
numTok = tok->tokAt(4);
415-
opTok = tok->tokAt(3);
416-
if (Token::Match(opTok, "<|>"))
417-
op = opTok->str()[0];
418-
} else if (Token::Match(tok, "%any% %comp% ( %any% &&|%oror%")) {
419-
numTok = tok;
420-
opTok = tok->next();
421-
if (Token::Match(opTok, "<|>"))
422-
op = opTok->str()[0]=='>'?'<':'>';
423-
}
424-
425-
else if (Token::Match(tok, "! %var% %comp% %any%") && !isNonBoolLHSExpr(tok)) {
426-
numTok = tok->tokAt(3);
427-
opTok = tok->tokAt(2);
428-
if (Token::Match(opTok, "<|>"))
429-
op = opTok->str()[0];
430-
} else if (Token::Match(tok->previous(), "(|&&|%oror% %num% %comp% !")) {
431-
const Token *rhs = tok->tokAt(3);
432-
while (rhs) {
433-
if (rhs->str() == "!") {
434-
if (Token::simpleMatch(rhs, "! ("))
435-
rhs = rhs->next()->link();
436-
rhs = rhs->next();
437-
} else if (rhs->isName() || rhs->isNumber())
438-
rhs = rhs->next();
439-
else
440-
break;
441-
}
442-
if (Token::Match(rhs, "&&|%oror%|)")) {
443-
numTok = tok;
444-
opTok = tok->next();
445-
if (Token::Match(opTok, "<|>"))
446-
op = opTok->str()[0]=='>'?'<':'>';
447-
}
383+
const Token* boolExpr = 0;
384+
bool numInRhs;
385+
if (astIsBool(tok->astOperand1())) {
386+
boolExpr = tok->astOperand1();
387+
numTok = tok->astOperand2();
388+
numInRhs = true;
389+
} else if (astIsBool(tok->astOperand2())) {
390+
boolExpr = tok->astOperand2();
391+
numTok = tok->astOperand1();
392+
numInRhs = false;
393+
} else {
394+
continue;
448395
}
449396

450-
// boolean result in lhs compared with <|<=|>|>=
451-
else if (tok->isComparisonOp() && !Token::Match(tok,"==|!=") && !isNonBoolLHSExpr(tok->previous())) {
452-
const Token *lhs = tok;
453-
while (nullptr != (lhs = lhs->previous())) {
454-
if ((lhs->isName() && !Token::Match(lhs,"or|and")) || lhs->isNumber())
455-
continue;
456-
if (lhs->isArithmeticalOp())
457-
continue;
458-
if (Token::Match(lhs, ")|]")) {
459-
if (Token::Match(lhs->link()->previous(), "%var% ("))
460-
lhs = lhs->link();
461-
continue;
462-
}
463-
break;
464-
}
465-
if (lhs && (lhs->isComparisonOp() || lhs->str() == "!")) {
466-
if (_tokenizer->isCPP() && tok->str() == ">" &&
467-
(Token::Match(lhs->previous(), "%var% <") || lhs->str() == ">"))
468-
continue;
469-
while (nullptr != (lhs = lhs->previous())) {
470-
if ((lhs->isName() && lhs->str() != "return") || lhs->isNumber())
471-
continue;
472-
if (Token::Match(lhs,"[+-*/.]"))
473-
continue;
474-
if (Token::Match(lhs, ")|]")) {
475-
lhs = lhs->previous();
476-
continue;
477-
}
478-
break;
479-
}
397+
if (Token::Match(boolExpr,"%bool%"))
398+
// The CheckBool::checkComparisonOfBoolWithInt warns about this.
399+
continue;
480400

481-
std::string expression;
482-
for (const Token *t = lhs ? lhs->next() : _tokenizer->tokens(); t != tok; t = t->next()) {
483-
if (!expression.empty())
484-
expression += ' ';
485-
expression += t->str();
486-
}
401+
if (!numTok || !boolExpr)
402+
continue;
487403

488-
comparisonOfBoolWithInvalidComparator(tok, expression);
489-
}
490-
}
404+
if (boolExpr->isOp() && numTok->isName() && Token::Match(tok, "==|!="))
405+
// there is weird code such as: ((a<b)==c)
406+
// but it is probably written this way by design.
407+
continue;
491408

492-
if (numTok && opTok) {
493-
if (numTok->isNumber()) {
494-
if (((numTok->str() != "0" && numTok->str() != "1") || !Token::Match(opTok, "!=|==")) && !((op == '<' && numTok->str() == "1") || (op == '>' && numTok->str() == "0")))
495-
comparisonOfBoolExpressionWithIntError(tok, true);
496-
} else if (isNonBoolStdType(numTok->variable()))
497-
comparisonOfBoolExpressionWithIntError(tok, false);
498-
}
409+
if (numTok->isNumber()) {
410+
if (numTok->str() == "0" && Token::Match(tok, numInRhs ? ">|==|!=" : "<|==|!="))
411+
continue;
412+
if (numTok->str() == "1" && Token::Match(tok, numInRhs ? "<|==|!=" : ">|==|!="))
413+
continue;
414+
comparisonOfBoolExpressionWithIntError(tok, true);
415+
} else if (isNonBoolStdType(numTok->variable()))
416+
comparisonOfBoolExpressionWithIntError(tok, false);
499417
}
500418
}
501419
}

‎test/testbool.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,7 @@ class TestBool : public TestFixture {
423423
ASSERT_EQUALS("",errout.str());
424424

425425
check("void f(int a, int b, int c) { if (1 < !(a+b)) {} }");
426-
TODO_ASSERT_EQUALS("error","",errout.str());
426+
ASSERT_EQUALS("[test.cpp:1]: (warning) Comparison of a boolean expression with an integer other than 0 or 1.\n",errout.str());
427427
}
428428

429429
void comparisonOfBoolExpressionWithInt3() {
@@ -438,12 +438,12 @@ class TestBool : public TestFixture {
438438
check("void f() {\n"
439439
" for(int i = 4; i > -1 < 5 ; --i) {}\n"
440440
"}");
441-
ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean value using relational operator (<, >, <= or >=).\n", errout.str());
441+
ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean expression with an integer other than 0 or 1.\n", errout.str());
442442

443443
check("void f(int a, int b, int c) {\n"
444444
" return (a > b) < c;\n"
445445
"}");
446-
TODO_ASSERT_EQUALS("error", "", errout.str());
446+
ASSERT_EQUALS("[test.cpp:2]: (warning) Comparison of a boolean expression with an integer.\n", errout.str());
447447

448448
check("void f(int a, int b, int c) {\n"
449449
" return x(a > b) < c;\n"

0 commit comments

Comments
 (0)