Skip to content

Commit 09a83f2

Browse files
bartlomiejgrzeskowiakdanmar
bartlomiejgrzeskowiak
authored and
danmar
committed
Fixed #7567 ("(a | 7) > 6U" is always true)
1 parent f337838 commit 09a83f2

2 files changed

Lines changed: 107 additions & 123 deletions

File tree

‎lib/checkcondition.cpp‎

Lines changed: 48 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -281,36 +281,55 @@ void CheckCondition::comparison()
281281
return;
282282

283283
for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) {
284-
if (Token::Match(tok, "==|!=|>")) {
285-
const Token *expr1 = tok->astOperand1();
286-
const Token *expr2 = tok->astOperand2();
287-
if (!expr1 || !expr2)
288-
continue;
289-
if (expr1->isNumber())
290-
std::swap(expr1,expr2);
291-
if (!expr2->isNumber())
292-
continue;
293-
const MathLib::bigint num2 = MathLib::toLongNumber(expr2->str());
294-
if (num2 < 0)
295-
continue;
296-
if (!Token::Match(expr1,"[&|]"))
284+
if (!tok->isComparisonOp())
285+
continue;
286+
287+
const Token *expr1 = tok->astOperand1();
288+
const Token *expr2 = tok->astOperand2();
289+
if (!expr1 || !expr2)
290+
continue;
291+
if (expr1->isNumber())
292+
std::swap(expr1,expr2);
293+
if (!expr2->isNumber())
294+
continue;
295+
const MathLib::bigint num2 = MathLib::toLongNumber(expr2->str());
296+
if (num2 < 0)
297+
continue;
298+
if (!Token::Match(expr1,"[&|]"))
299+
continue;
300+
std::list<MathLib::bigint> numbers;
301+
getnumchildren(expr1, numbers);
302+
for (std::list<MathLib::bigint>::const_iterator num = numbers.begin(); num != numbers.end(); ++num) {
303+
const MathLib::bigint num1 = *num;
304+
if (num1 < 0)
297305
continue;
298-
std::list<MathLib::bigint> numbers;
299-
getnumchildren(expr1, numbers);
300-
for (std::list<MathLib::bigint>::const_iterator num = numbers.begin(); num != numbers.end(); ++num) {
301-
const MathLib::bigint num1 = *num;
302-
if (num1 < 0)
303-
continue;
304-
if (Token::Match(tok, "==|!=")) {
305-
if ((expr1->str() == "&" && (num1 & num2) != num2) ||
306-
(expr1->str() == "|" && (num1 | num2) != num2)) {
307-
const std::string& op(tok->str());
308-
comparisonError(expr1, expr1->str(), num1, op, num2, op=="==" ? false : true);
309-
}
310-
} else if (Token::simpleMatch(tok, ">")) {
311-
if ((expr1->str() == "&" && (num1 <= num2))) {
312-
const std::string& op(tok->str());
313-
comparisonError(expr1, expr1->str(), num1, op, num2, false);
306+
if (Token::Match(tok, "==|!=")) {
307+
if ((expr1->str() == "&" && (num1 & num2) != num2) ||
308+
(expr1->str() == "|" && (num1 | num2) != num2)) {
309+
const std::string& op(tok->str());
310+
comparisonError(expr1, expr1->str(), num1, op, num2, op=="==" ? false : true);
311+
}
312+
} else if (expr1->str() == "&") {
313+
const bool or_equal = Token::Match(tok, ">=|<=");
314+
const std::string& op(tok->str());
315+
if ((Token::Match(tok, ">=|<")) && (num1 < num2)) {
316+
comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? false : true);
317+
} else if ((Token::Match(tok, "<=|>")) && (num1 <= num2)) {
318+
comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? true : false);
319+
}
320+
} else if (expr1->str() == "|") {
321+
if ((expr1->astOperand1()->valueType()) &&
322+
(expr1->astOperand1()->valueType()->sign == ValueType::Sign::UNSIGNED)) {
323+
const bool or_equal = Token::Match(tok, ">=|<=");
324+
const std::string& op(tok->str());
325+
if ((Token::Match(tok, ">=|<")) && (num1 >= num2)) {
326+
//"(a | 0x07) >= 7U" is always true for unsigned a
327+
//"(a | 0x07) < 7U" is always false for unsigned a
328+
comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? true : false);
329+
} else if ((Token::Match(tok, "<=|>")) && (num1 > num2)) {
330+
//"(a | 0x08) <= 7U" is always false for unsigned a
331+
//"(a | 0x07) > 6U" is always true for unsigned a
332+
comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? false : true);
314333
}
315334
}
316335
}

‎test/testcondition.cpp‎

Lines changed: 59 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ class TestCondition : public TestFixture {
4747

4848
TEST_CASE(assignAndCompare); // assignment and comparison don't match
4949
TEST_CASE(mismatchingBitAnd); // overlapping bitmasks
50-
TEST_CASE(compare); // mismatching LHS/RHS in comparison
50+
TEST_CASE(comparison); // CheckCondition::comparison test cases
5151
TEST_CASE(multicompare); // mismatching comparisons
5252
TEST_CASE(duplicateIf); // duplicate conditions in if and else-if
5353

@@ -318,30 +318,64 @@ class TestCondition : public TestFixture {
318318
ASSERT_EQUALS("", errout.str());
319319
}
320320

321-
void compare() {
322-
check("void foo(int x)\n"
323-
"{\n"
324-
" if ((x & 4) == 3);\n"
325-
"}");
326-
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str());
327-
328-
check("void foo(int x)\n"
329-
"{\n"
330-
" if ((x | 4) == 3);\n"
331-
"}");
332-
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X | 0x4) == 0x3' is always false.\n", errout.str());
333-
334-
check("void foo(int x)\n"
335-
"{\n"
336-
" if ((x | 4) != 3);\n"
337-
"}");
338-
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X | 0x4) != 0x3' is always true.\n", errout.str());
339-
340-
check("void foo(int x)\n"
341-
"{\n"
342-
" if ((x & y & 4 & z ) == 3);\n"
343-
"}");
344-
ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str());
321+
void comparison() {
322+
// CheckCondition::comparison test cases
323+
// '=='
324+
check("void f(int a) {\n assert( (a & 0x07) == 8U );\n}");
325+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) == 0x8' is always false.\n",errout.str());
326+
check("void f(int a) {\n assert( (a & b & 4 & c ) == 3 );\n}");
327+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str());
328+
check("void f(int a) {\n assert( (a | 0x07) == 8U );\n}");
329+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) == 0x8' is always false.\n",errout.str());
330+
check("void f(int a) {\n assert( (a & 0x07) == 7U );\n}");
331+
ASSERT_EQUALS("", errout.str());
332+
check("void f(int a) {\n assert( (a | 0x01) == -15 );\n}");
333+
ASSERT_EQUALS("", errout.str());
334+
// '!='
335+
check("void f(int a) {\n assert( (a & 0x07) != 8U );\n}");
336+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) != 0x8' is always true.\n",errout.str());
337+
check("void f(int a) {\n assert( (a | 0x07) != 8U );\n}");
338+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) != 0x8' is always true.\n",errout.str());
339+
check("void f(int a) {\n assert( (a & 0x07) != 7U );\n}");
340+
ASSERT_EQUALS("", errout.str());
341+
check("void f(int a) {\n assert( (a | 0x07) != 7U );\n}");
342+
ASSERT_EQUALS("", errout.str());
343+
// '>='
344+
check("void f(int a) {\n assert( (a & 0x07) >= 8U );\n}");
345+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) >= 0x8' is always false.\n",errout.str());
346+
check("void f(unsigned int a) {\n assert( (a | 0x7) >= 7U );\n}");
347+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) >= 0x7' is always true.\n",errout.str());
348+
check("void f(int a) {\n assert( (a & 0x07) >= 7U );\n}");
349+
ASSERT_EQUALS("",errout.str());
350+
check("void f(int a) {\n assert( (a | 0x07) >= 8U );\n}");
351+
ASSERT_EQUALS("",errout.str()); //correct for negative 'a'
352+
// '>'
353+
check("void f(int a) {\n assert( (a & 0x07) > 7U );\n}");
354+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) > 0x7' is always false.\n",errout.str());
355+
check("void f(unsigned int a) {\n assert( (a | 0x7) > 6U );\n}");
356+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) > 0x6' is always true.\n",errout.str());
357+
check("void f(int a) {\n assert( (a & 0x07) > 6U );\n}");
358+
ASSERT_EQUALS("",errout.str());
359+
check("void f(int a) {\n assert( (a | 0x07) > 7U );\n}");
360+
ASSERT_EQUALS("",errout.str()); //correct for negative 'a'
361+
// '<='
362+
check("void f(int a) {\n assert( (a & 0x07) <= 7U );\n}");
363+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) <= 0x7' is always true.\n",errout.str());
364+
check("void f(unsigned int a) {\n assert( (a | 0x08) <= 7U );\n}");
365+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x8) <= 0x7' is always false.\n",errout.str());
366+
check("void f(int a) {\n assert( (a & 0x07) <= 6U );\n}");
367+
ASSERT_EQUALS("",errout.str());
368+
check("void f(int a) {\n assert( (a | 0x08) <= 7U );\n}");
369+
ASSERT_EQUALS("",errout.str()); //correct for negative 'a'
370+
// '<'
371+
check("void f(int a) {\n assert( (a & 0x07) < 8U );\n}");
372+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) < 0x8' is always true.\n",errout.str());
373+
check("void f(unsigned int a) {\n assert( (a | 0x07) < 7U );\n}");
374+
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) < 0x7' is always false.\n",errout.str());
375+
check("void f(int a) {\n assert( (a & 0x07) < 3U );\n}");
376+
ASSERT_EQUALS("",errout.str());
377+
check("void f(int a) {\n assert( (a | 0x07) < 7U );\n}");
378+
ASSERT_EQUALS("",errout.str()); //correct for negative 'a'
345379
}
346380

347381
void multicompare() {
@@ -592,75 +626,6 @@ class TestCondition : public TestFixture {
592626
" }\n"
593627
"}");
594628
ASSERT_EQUALS("", errout.str());
595-
596-
//Various bitmask comparison checks
597-
//#7428 false negative: condition '(a&7)>7U' is always false
598-
//#7522 false positive: condition '(X|7)>=6' is correct
599-
600-
check("void f() {\n"
601-
"assert( (a & 0x07) == 8U );\n" // statement always false
602-
"}");
603-
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) == 0x8' is always false.\n",
604-
errout.str());
605-
606-
check("void f() {\n"
607-
"assert( (a & 0x07) == 7U );\n" // statement correct
608-
"assert( (a & 0x07) == 6U );\n" // statement correct
609-
"}");
610-
ASSERT_EQUALS("", errout.str());
611-
612-
check("void f() {\n"
613-
"assert( (a | 0x07) == 8U );\n" // statement always false
614-
"}");
615-
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) == 0x8' is always false.\n",
616-
errout.str());
617-
618-
check("void f() {\n"
619-
"assert( (a | 0x07) == 7U );\n" // statement correct
620-
"assert( (a | 0x07) == 23U );\n" // statement correct
621-
"}");
622-
ASSERT_EQUALS("", errout.str());
623-
624-
check("void f() {\n"
625-
"assert( (a & 0x07) != 8U );\n" // statement always true
626-
"}");
627-
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) != 0x8' is always true.\n",
628-
errout.str());
629-
630-
check("void f() {\n"
631-
"assert( (a & 0x07) != 7U );\n" // statement correct
632-
"assert( (a & 0x07) != 0U );\n" // statement correct
633-
"}");
634-
ASSERT_EQUALS("", errout.str());
635-
636-
check("void f() {\n"
637-
"assert( (a | 0x07) != 8U );\n" // statement always true
638-
"}");
639-
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) != 0x8' is always true.\n",
640-
errout.str());
641-
642-
check("void f() {\n"
643-
"assert( (a | 0x07) != 7U );\n" // statement correct
644-
"}");
645-
ASSERT_EQUALS("", errout.str());
646-
647-
//TRAC #7428 false negative: condition '(X&7)>7' is always false
648-
check("void f() {\n"
649-
"assert( (a & 0x07) > 7U );\n" // statement always false
650-
"}");
651-
ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) > 0x7' is always false.\n",
652-
errout.str());
653-
654-
check("void f() {\n"
655-
"assert( (a & 0x07) > 6U );\n" // statement correct
656-
"}");
657-
ASSERT_EQUALS("", errout.str());
658-
659-
//TRAC #7522 false positive: condition '(X|7)>=6' is correct (X can be negative)
660-
check("void f() {\n"
661-
"assert( (a | 0x07) >= 6U );\n" // statement correct (X can be negative)
662-
"}");
663-
ASSERT_EQUALS("",errout.str());
664629
}
665630

666631

0 commit comments

Comments
 (0)