diff --git a/lib/checkcondition.cpp b/lib/checkcondition.cpp index 6fc86e3ea..033ec51d4 100644 --- a/lib/checkcondition.cpp +++ b/lib/checkcondition.cpp @@ -281,36 +281,55 @@ void CheckCondition::comparison() return; for (const Token *tok = _tokenizer->tokens(); tok; tok = tok->next()) { - if (Token::Match(tok, "==|!=|>")) { - const Token *expr1 = tok->astOperand1(); - const Token *expr2 = tok->astOperand2(); - if (!expr1 || !expr2) + if (!tok->isComparisonOp()) + continue; + + const Token *expr1 = tok->astOperand1(); + const Token *expr2 = tok->astOperand2(); + if (!expr1 || !expr2) + continue; + if (expr1->isNumber()) + std::swap(expr1,expr2); + if (!expr2->isNumber()) + continue; + const MathLib::bigint num2 = MathLib::toLongNumber(expr2->str()); + if (num2 < 0) + continue; + if (!Token::Match(expr1,"[&|]")) + continue; + std::list numbers; + getnumchildren(expr1, numbers); + for (std::list::const_iterator num = numbers.begin(); num != numbers.end(); ++num) { + const MathLib::bigint num1 = *num; + if (num1 < 0) continue; - if (expr1->isNumber()) - std::swap(expr1,expr2); - if (!expr2->isNumber()) - continue; - const MathLib::bigint num2 = MathLib::toLongNumber(expr2->str()); - if (num2 < 0) - continue; - if (!Token::Match(expr1,"[&|]")) - continue; - std::list numbers; - getnumchildren(expr1, numbers); - for (std::list::const_iterator num = numbers.begin(); num != numbers.end(); ++num) { - const MathLib::bigint num1 = *num; - if (num1 < 0) - continue; - if (Token::Match(tok, "==|!=")) { - if ((expr1->str() == "&" && (num1 & num2) != num2) || - (expr1->str() == "|" && (num1 | num2) != num2)) { - const std::string& op(tok->str()); - comparisonError(expr1, expr1->str(), num1, op, num2, op=="==" ? false : true); - } - } else if (Token::simpleMatch(tok, ">")) { - if ((expr1->str() == "&" && (num1 <= num2))) { - const std::string& op(tok->str()); - comparisonError(expr1, expr1->str(), num1, op, num2, false); + if (Token::Match(tok, "==|!=")) { + if ((expr1->str() == "&" && (num1 & num2) != num2) || + (expr1->str() == "|" && (num1 | num2) != num2)) { + const std::string& op(tok->str()); + comparisonError(expr1, expr1->str(), num1, op, num2, op=="==" ? false : true); + } + } else if (expr1->str() == "&") { + const bool or_equal = Token::Match(tok, ">=|<="); + const std::string& op(tok->str()); + if ((Token::Match(tok, ">=|<")) && (num1 < num2)) { + comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? false : true); + } else if ((Token::Match(tok, "<=|>")) && (num1 <= num2)) { + comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? true : false); + } + } else if (expr1->str() == "|") { + if ((expr1->astOperand1()->valueType()) && + (expr1->astOperand1()->valueType()->sign == ValueType::Sign::UNSIGNED)) { + const bool or_equal = Token::Match(tok, ">=|<="); + const std::string& op(tok->str()); + if ((Token::Match(tok, ">=|<")) && (num1 >= num2)) { + //"(a | 0x07) >= 7U" is always true for unsigned a + //"(a | 0x07) < 7U" is always false for unsigned a + comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? true : false); + } else if ((Token::Match(tok, "<=|>")) && (num1 > num2)) { + //"(a | 0x08) <= 7U" is always false for unsigned a + //"(a | 0x07) > 6U" is always true for unsigned a + comparisonError(expr1, expr1->str(), num1, op, num2, or_equal ? false : true); } } } diff --git a/test/testcondition.cpp b/test/testcondition.cpp index 3dfd7e66a..5bc4f1f49 100644 --- a/test/testcondition.cpp +++ b/test/testcondition.cpp @@ -47,7 +47,7 @@ private: TEST_CASE(assignAndCompare); // assignment and comparison don't match TEST_CASE(mismatchingBitAnd); // overlapping bitmasks - TEST_CASE(compare); // mismatching LHS/RHS in comparison + TEST_CASE(comparison); // CheckCondition::comparison test cases TEST_CASE(multicompare); // mismatching comparisons TEST_CASE(duplicateIf); // duplicate conditions in if and else-if @@ -318,30 +318,64 @@ private: ASSERT_EQUALS("", errout.str()); } - void compare() { - check("void foo(int x)\n" - "{\n" - " if ((x & 4) == 3);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str()); - - check("void foo(int x)\n" - "{\n" - " if ((x | 4) == 3);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X | 0x4) == 0x3' is always false.\n", errout.str()); - - check("void foo(int x)\n" - "{\n" - " if ((x | 4) != 3);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X | 0x4) != 0x3' is always true.\n", errout.str()); - - check("void foo(int x)\n" - "{\n" - " if ((x & y & 4 & z ) == 3);\n" - "}"); - ASSERT_EQUALS("[test.cpp:3]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str()); + void comparison() { + // CheckCondition::comparison test cases + // '==' + check("void f(int a) {\n assert( (a & 0x07) == 8U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) == 0x8' is always false.\n",errout.str()); + check("void f(int a) {\n assert( (a & b & 4 & c ) == 3 );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x4) == 0x3' is always false.\n", errout.str()); + check("void f(int a) {\n assert( (a | 0x07) == 8U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) == 0x8' is always false.\n",errout.str()); + check("void f(int a) {\n assert( (a & 0x07) == 7U );\n}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int a) {\n assert( (a | 0x01) == -15 );\n}"); + ASSERT_EQUALS("", errout.str()); + // '!=' + check("void f(int a) {\n assert( (a & 0x07) != 8U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) != 0x8' is always true.\n",errout.str()); + check("void f(int a) {\n assert( (a | 0x07) != 8U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) != 0x8' is always true.\n",errout.str()); + check("void f(int a) {\n assert( (a & 0x07) != 7U );\n}"); + ASSERT_EQUALS("", errout.str()); + check("void f(int a) {\n assert( (a | 0x07) != 7U );\n}"); + ASSERT_EQUALS("", errout.str()); + // '>=' + check("void f(int a) {\n assert( (a & 0x07) >= 8U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) >= 0x8' is always false.\n",errout.str()); + check("void f(unsigned int a) {\n assert( (a | 0x7) >= 7U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) >= 0x7' is always true.\n",errout.str()); + check("void f(int a) {\n assert( (a & 0x07) >= 7U );\n}"); + ASSERT_EQUALS("",errout.str()); + check("void f(int a) {\n assert( (a | 0x07) >= 8U );\n}"); + ASSERT_EQUALS("",errout.str()); //correct for negative 'a' + // '>' + check("void f(int a) {\n assert( (a & 0x07) > 7U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) > 0x7' is always false.\n",errout.str()); + check("void f(unsigned int a) {\n assert( (a | 0x7) > 6U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) > 0x6' is always true.\n",errout.str()); + check("void f(int a) {\n assert( (a & 0x07) > 6U );\n}"); + ASSERT_EQUALS("",errout.str()); + check("void f(int a) {\n assert( (a | 0x07) > 7U );\n}"); + ASSERT_EQUALS("",errout.str()); //correct for negative 'a' + // '<=' + check("void f(int a) {\n assert( (a & 0x07) <= 7U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) <= 0x7' is always true.\n",errout.str()); + check("void f(unsigned int a) {\n assert( (a | 0x08) <= 7U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x8) <= 0x7' is always false.\n",errout.str()); + check("void f(int a) {\n assert( (a & 0x07) <= 6U );\n}"); + ASSERT_EQUALS("",errout.str()); + check("void f(int a) {\n assert( (a | 0x08) <= 7U );\n}"); + ASSERT_EQUALS("",errout.str()); //correct for negative 'a' + // '<' + check("void f(int a) {\n assert( (a & 0x07) < 8U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) < 0x8' is always true.\n",errout.str()); + check("void f(unsigned int a) {\n assert( (a | 0x07) < 7U );\n}"); + ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) < 0x7' is always false.\n",errout.str()); + check("void f(int a) {\n assert( (a & 0x07) < 3U );\n}"); + ASSERT_EQUALS("",errout.str()); + check("void f(int a) {\n assert( (a | 0x07) < 7U );\n}"); + ASSERT_EQUALS("",errout.str()); //correct for negative 'a' } void multicompare() { @@ -592,75 +626,6 @@ private: " }\n" "}"); ASSERT_EQUALS("", errout.str()); - - //Various bitmask comparison checks - //#7428 false negative: condition '(a&7)>7U' is always false - //#7522 false positive: condition '(X|7)>=6' is correct - - check("void f() {\n" - "assert( (a & 0x07) == 8U );\n" // statement always false - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) == 0x8' is always false.\n", - errout.str()); - - check("void f() {\n" - "assert( (a & 0x07) == 7U );\n" // statement correct - "assert( (a & 0x07) == 6U );\n" // statement correct - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f() {\n" - "assert( (a | 0x07) == 8U );\n" // statement always false - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) == 0x8' is always false.\n", - errout.str()); - - check("void f() {\n" - "assert( (a | 0x07) == 7U );\n" // statement correct - "assert( (a | 0x07) == 23U );\n" // statement correct - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f() {\n" - "assert( (a & 0x07) != 8U );\n" // statement always true - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) != 0x8' is always true.\n", - errout.str()); - - check("void f() {\n" - "assert( (a & 0x07) != 7U );\n" // statement correct - "assert( (a & 0x07) != 0U );\n" // statement correct - "}"); - ASSERT_EQUALS("", errout.str()); - - check("void f() {\n" - "assert( (a | 0x07) != 8U );\n" // statement always true - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X | 0x7) != 0x8' is always true.\n", - errout.str()); - - check("void f() {\n" - "assert( (a | 0x07) != 7U );\n" // statement correct - "}"); - ASSERT_EQUALS("", errout.str()); - - //TRAC #7428 false negative: condition '(X&7)>7' is always false - check("void f() {\n" - "assert( (a & 0x07) > 7U );\n" // statement always false - "}"); - ASSERT_EQUALS("[test.cpp:2]: (style) Expression '(X & 0x7) > 0x7' is always false.\n", - errout.str()); - - check("void f() {\n" - "assert( (a & 0x07) > 6U );\n" // statement correct - "}"); - ASSERT_EQUALS("", errout.str()); - - //TRAC #7522 false positive: condition '(X|7)>=6' is correct (X can be negative) - check("void f() {\n" - "assert( (a | 0x07) >= 6U );\n" // statement correct (X can be negative) - "}"); - ASSERT_EQUALS("",errout.str()); }