Skip to content

Commit d777511

Browse files
firewavepfultz2
firewave
and
pfultz2
authored
fixed #11202/#11199/#11201/#12330/#12774 / refs #13508/#11200/#13506 - fixed various floating-point comparison regressions (danmar#7148)
Co-authored-by: Paul Fultz II <[email protected]>
1 parent 74da655 commit d777511

File tree

5 files changed

+267
-11
lines changed

5 files changed

+267
-11
lines changed

‎lib/calculate.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,14 +63,14 @@ R calculate(const std::string& s, const T& x, const T& y, bool* error = nullptr)
6363
case '*':
6464
return wrap(x * y);
6565
case '/':
66-
if (isZero(y) || (std::is_integral<T>{} && std::is_signed<T>{} && isEqual(y, T(-1)) && isEqual(x, std::numeric_limits<T>::min()))) {
66+
if (isZero(y) || (std::is_signed<T>{} && y < 0)) {
6767
if (error)
6868
*error = true;
6969
return R{};
7070
}
7171
return wrap(x / y);
7272
case '%':
73-
if (isZero(MathLib::bigint(y)) || (std::is_integral<T>{} && std::is_signed<T>{} && isEqual(y, T(-1)) && isEqual(x, std::numeric_limits<T>::min()))) {
73+
if (isZero(MathLib::bigint(y)) || (std::is_signed<T>{} && MathLib::bigint(y) < 0)) {
7474
if (error)
7575
*error = true;
7676
return R{};

‎lib/vf_settokenvalue.cpp‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -496,6 +496,7 @@ namespace ValueFlow
496496
}
497497
const double floatValue1 = value1.isFloatValue() ? value1.floatValue : static_cast<double>(value1.intvalue);
498498
const double floatValue2 = value2.isFloatValue() ? value2.floatValue : static_cast<double>(value2.intvalue);
499+
const bool isFloat = value1.isFloatValue() || value2.isFloatValue();
499500
const auto intValue1 = [&]() -> MathLib::bigint {
500501
return value1.isFloatValue() ? static_cast<MathLib::bigint>(value1.floatValue) : value1.intvalue;
501502
};
@@ -550,17 +551,27 @@ namespace ValueFlow
550551
setTokenValue(parent, std::move(result), settings);
551552
} else if (Token::Match(parent, "%op%")) {
552553
if (Token::Match(parent, "%comp%")) {
553-
if (!result.isFloatValue() && !value1.isIntValue() && !value2.isIntValue())
554+
if (!isFloat && !value1.isIntValue() && !value2.isIntValue())
554555
continue;
555556
} else {
556557
if (value1.isTokValue() || value2.isTokValue())
557558
break;
558559
}
559560
bool error = false;
560-
if (result.isFloatValue()) {
561-
result.floatValue = calculate(parent->str(), floatValue1, floatValue2, &error);
561+
if (isFloat) {
562+
auto val = calculate(parent->str(), floatValue1, floatValue2, &error);
563+
if (result.isFloatValue()) {
564+
result.floatValue = val;
565+
} else {
566+
result.intvalue = static_cast<MathLib::bigint>(val);
567+
}
562568
} else {
563-
result.intvalue = calculate(parent->str(), intValue1(), intValue2(), &error);
569+
auto val = calculate(parent->str(), intValue1(), intValue2(), &error);
570+
if (result.isFloatValue()) {
571+
result.floatValue = static_cast<double>(val);
572+
} else {
573+
result.intvalue = val;
574+
}
564575
}
565576
if (error)
566577
continue;

‎test/testcondition.cpp‎

Lines changed: 129 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,7 @@ class TestCondition : public TestFixture {
124124
TEST_CASE(knownConditionIncrementLoop); // #9808
125125
TEST_CASE(knownConditionAfterBailout); // #12526
126126
TEST_CASE(knownConditionIncDecOperator);
127+
TEST_CASE(knownConditionFloating);
127128
}
128129

129130
#define check(...) check_(__FILE__, __LINE__, __VA_ARGS__)
@@ -4464,9 +4465,10 @@ class TestCondition : public TestFixture {
44644465
" float f = 9.9f;\n"
44654466
" if(f < 10) {}\n"
44664467
"}\n");
4467-
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'i>9.9' is always true\n"
4468-
"[test.cpp:5]: (style) Condition 'f<10' is always true\n",
4469-
errout_str());
4468+
ASSERT_EQUALS(
4469+
"[test.cpp:3]: (style) Condition 'i>9.9' is always true\n"
4470+
"[test.cpp:5]: (style) Condition 'f<10' is always true\n",
4471+
errout_str());
44704472
check("constexpr int f() {\n" // #11238
44714473
" return 1;\n"
44724474
"}\n"
@@ -5770,6 +5772,14 @@ class TestCondition : public TestFixture {
57705772
" if (other.mPA.cols > 0 && other.mPA.rows > 0)\n"
57715773
" ;\n"
57725774
"}");
5775+
ASSERT_EQUALS("", errout_str());
5776+
5777+
check("void foo() {\n" // #11202
5778+
" float f = 0x1.4p+3;\n"
5779+
" if (f > 10.0) {}\n"
5780+
" if (f < 10.0) {}\n"
5781+
"}\n");
5782+
ASSERT_EQUALS("", errout_str());
57735783
}
57745784

57755785
void checkInvalidTestForOverflow() {
@@ -6229,6 +6239,122 @@ class TestCondition : public TestFixture {
62296239
"}\n");
62306240
ASSERT_EQUALS("", errout_str());
62316241
}
6242+
6243+
void knownConditionFloating() {
6244+
check("void foo() {\n" // #11199
6245+
" float f = 1.0;\n"
6246+
" if (f > 1.0f) {}\n"
6247+
"}\n");
6248+
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0f' is always false\n", errout_str());
6249+
6250+
check("void foo() {\n" // #11199
6251+
" float f = 1.0;\n"
6252+
" if (f > 1.0L) {}\n"
6253+
"}\n");
6254+
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0L' is always false\n", errout_str());
6255+
6256+
check("void foo() {\n" // #11199
6257+
" float f = 1.0f;\n"
6258+
" if (f > 1.0) {}\n"
6259+
"}\n");
6260+
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0' is always false\n", errout_str());
6261+
6262+
check("void foo() {\n" // #11199
6263+
" float f = 1.0f;\n"
6264+
" if (f > 1.0L) {}\n"
6265+
"}\n");
6266+
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0L' is always false\n", errout_str());
6267+
6268+
check("void foo() {\n" // #11199
6269+
" float f = 1.0L;\n"
6270+
" if (f > 1.0) {}\n"
6271+
"}\n");
6272+
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0' is always false\n", errout_str());
6273+
6274+
check("void foo() {\n" // #11199
6275+
" float f = 1.0L;\n"
6276+
" if (f > 1.0f) {}\n"
6277+
"}\n");
6278+
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f>1.0f' is always false\n", errout_str());
6279+
6280+
check("void foo() {\n" // #11201
6281+
" float f = 0x1.4p+3;\n" // hex fraction 1.4 (decimal 1.25) scaled by 2^3, that is 10.0
6282+
" if (f > 9.9) {}\n"
6283+
" if (f < 9.9) {}\n"
6284+
"}\n");
6285+
ASSERT_EQUALS(
6286+
"[test.cpp:3]: (style) Condition 'f>9.9' is always true\n"
6287+
"[test.cpp:4]: (style) Condition 'f<9.9' is always false\n",
6288+
errout_str());
6289+
6290+
check("void foo() {\n" // #12330
6291+
" double d = 1.0;\n"
6292+
" if (d < 0.0) {}\n"
6293+
"}\n");
6294+
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'd<0.0' is always false\n", errout_str());
6295+
6296+
check("void foo() {\n" // #12330
6297+
" long double ld = 1.0;\n"
6298+
" if (ld < 0.0) {}\n"
6299+
"}\n");
6300+
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'ld<0.0' is always false\n", errout_str());
6301+
6302+
check("void foo() {\n" // #12330
6303+
" float f = 1.0;\n"
6304+
" if (f < 0.0) {}\n"
6305+
"}\n");
6306+
ASSERT_EQUALS("[test.cpp:3]: (style) Condition 'f<0.0' is always false\n", errout_str());
6307+
6308+
check("void foo() {\n" // #12774
6309+
" float f = 1.0f;\n"
6310+
" if (f > 1.01f) {}\n"
6311+
"}\n");
6312+
ASSERT_EQUALS(
6313+
"[test.cpp:3]: (style) Condition 'f>1.01f' is always false\n",
6314+
errout_str());
6315+
6316+
check("void foo() {\n" // #12774
6317+
" float f = 1.0;\n"
6318+
" if (f > 1.01) {}\n"
6319+
"}\n");
6320+
ASSERT_EQUALS(
6321+
"[test.cpp:3]: (style) Condition 'f>1.01' is always false\n",
6322+
errout_str());
6323+
6324+
check("void foo() {\n"
6325+
" float f = 1.0f;\n"
6326+
" if (f > 1) {}\n"
6327+
"}\n");
6328+
ASSERT_EQUALS(
6329+
"[test.cpp:3]: (style) Condition 'f>1' is always false\n",
6330+
errout_str());
6331+
6332+
check("void foo() {\n" // #13508
6333+
" float f = 1.0f;\n"
6334+
" if (f > 1.00f) {}\n"
6335+
"}\n");
6336+
TODO_ASSERT_EQUALS(
6337+
"[test.cpp:3]: (style) Condition 'f>1.00f' is always false\n",
6338+
"",
6339+
errout_str());
6340+
6341+
check("void foo() {\n"
6342+
" float f = 1.0;\n"
6343+
" if (f > 1) {}\n"
6344+
"}\n");
6345+
ASSERT_EQUALS(
6346+
"[test.cpp:3]: (style) Condition 'f>1' is always false\n",
6347+
errout_str());
6348+
6349+
check("void foo() {\n"// #13508
6350+
" float f = 1.0;\n"
6351+
" if (f > 1.00) {}\n"
6352+
"}\n");
6353+
TODO_ASSERT_EQUALS(
6354+
"[test.cpp:3]: (style) Condition 'f>1.00' is always false\n",
6355+
"",
6356+
errout_str());
6357+
}
62326358
};
62336359

62346360
REGISTER_TEST(TestCondition)

‎test/testother.cpp‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,8 @@ class TestOther : public TestFixture {
301301

302302
TEST_CASE(knownPointerToBool);
303303
TEST_CASE(iterateByValue);
304+
305+
TEST_CASE(alwaysTrueFloating);
304306
}
305307

306308
#define check(...) check_(__FILE__, __LINE__, __VA_ARGS__)
@@ -12885,6 +12887,103 @@ class TestOther : public TestFixture {
1288512887
ASSERT_EQUALS("[test.cpp:3]: (performance) Range variable 's' should be declared as const reference.\n",
1288612888
errout_str());
1288712889
}
12890+
12891+
void alwaysTrueFloating()
12892+
{
12893+
check("void foo() {\n" // #11200
12894+
" float f = 1.0;\n"
12895+
" if (f > 1.0) {}\n"
12896+
" if (f > -1.0) {}\n"
12897+
"}\n");
12898+
TODO_ASSERT_EQUALS(
12899+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1.0' is always false.\n"
12900+
"[test.cpp:2] -> [test.cpp:4]: (style) The comparison 'f > -1.0' is always false.\n",
12901+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1.0' is always false.\n",
12902+
errout_str());
12903+
12904+
check("void foo() {\n" // #13506
12905+
" float f = 1.0;\n"
12906+
" if (f > +1.0) {}\n"
12907+
"}\n");
12908+
TODO_ASSERT_EQUALS(
12909+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > +1.0' is always false.\n",
12910+
"",
12911+
errout_str());
12912+
12913+
check("void foo() {\n" // #11200
12914+
" float pf = +1.0;\n"
12915+
" if (pf > 1.0) {}\n"
12916+
" if (pf > -1.0) {}\n"
12917+
"}\n");
12918+
TODO_ASSERT_EQUALS(
12919+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'pf > 1.0' is always false.\n"
12920+
"[test.cpp:2] -> [test.cpp:4]: (style) The comparison 'pf > -1.0' is always false.\n",
12921+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'pf > 1.0' is always false.\n",
12922+
errout_str());
12923+
12924+
check("void foo() {\n" // #13506
12925+
" float pf = +1.0;\n"
12926+
" if (pf > +1.0) {}\n"
12927+
"}\n");
12928+
TODO_ASSERT_EQUALS(
12929+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'pf > +1.0' is always false.\n",
12930+
"",
12931+
errout_str());
12932+
12933+
check("void foo() {\n" // #11200
12934+
" float nf = -1.0;\n"
12935+
" if (nf > 1.0) {}\n"
12936+
" if (nf > -1.0) {}\n"
12937+
"}\n");
12938+
TODO_ASSERT_EQUALS(
12939+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'nf > 1.0' is always false.\n"
12940+
"[test.cpp:2] -> [test.cpp:4]: (style) The comparison 'nf > -1.0' is always false.\n",
12941+
"[test.cpp:2] -> [test.cpp:4]: (style) The comparison 'nf > -1.0' is always false.\n",
12942+
errout_str());
12943+
12944+
check("void foo() {\n" // #13506
12945+
" float nf = -1.0;\n"
12946+
" if (nf > +1.0) {}\n"
12947+
"}\n");
12948+
TODO_ASSERT_EQUALS(
12949+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'nf > +1.0' is always false.\n",
12950+
"",
12951+
errout_str());
12952+
12953+
check("void foo() {\n"
12954+
" float f = 1.0f;\n"
12955+
" if (f > 1.00f) {}\n"
12956+
"}\n");
12957+
ASSERT_EQUALS(
12958+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1.00f' is always false.\n",
12959+
errout_str());
12960+
12961+
check("void foo() {\n" // #13508
12962+
" float f = 1.0f;\n"
12963+
" if (f > 1) {}\n"
12964+
"}\n");
12965+
TODO_ASSERT_EQUALS(
12966+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1' is always false.\n",
12967+
"",
12968+
errout_str());
12969+
12970+
check("void foo() {\n"
12971+
" float f = 1.0;\n"
12972+
" if (f > 1.00) {}\n"
12973+
"}\n");
12974+
ASSERT_EQUALS(
12975+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1.00' is always false.\n",
12976+
errout_str());
12977+
12978+
check("void foo() {\n" // #13508
12979+
" float f = 1.0;\n"
12980+
" if (f > 1) {}\n"
12981+
"}\n");
12982+
TODO_ASSERT_EQUALS(
12983+
"[test.cpp:2] -> [test.cpp:3]: (style) The comparison 'f > 1' is always false.\n",
12984+
"",
12985+
errout_str());
12986+
}
1288812987
};
1288912988

1289012989
REGISTER_TEST(TestOther)

‎test/testvalueflow.cpp‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -951,15 +951,35 @@ class TestValueFlow : public TestFixture {
951951
ASSERT_EQUALS(10, valueOfTok("x = static_cast<int>(10);", "( 10 )").intvalue);
952952
ASSERT_EQUALS(0, valueOfTok("x = sizeof (struct {int a;}) * 0;", "*").intvalue);
953953

954-
// Don't calculate if there is UB
954+
// Don't calculate or crash if there is UB or invalid operations
955955
ASSERT(tokenValues(";-1<<10;","<<").empty());
956956
ASSERT(tokenValues(";10<<-1;","<<").empty());
957957
ASSERT(tokenValues(";10<<64;","<<").empty());
958958
ASSERT(tokenValues(";-1>>10;",">>").empty());
959959
ASSERT(tokenValues(";10>>-1;",">>").empty());
960960
ASSERT(tokenValues(";10>>64;",">>").empty());
961+
ASSERT_EQUALS(tokenValues(";1%-1;","%").size(), 1);
962+
ASSERT_EQUALS(tokenValues(";1%-10;","%").size(), 1);
963+
ASSERT_EQUALS(tokenValues(";1.5%-1;","%").size(), 1);
964+
ASSERT_EQUALS(tokenValues(";1.5%-10;","%").size(), 1);
965+
ASSERT(tokenValues(";1%-1.5;","%").empty());
966+
ASSERT(tokenValues(";1%-10.5;","%").empty());
967+
ASSERT(tokenValues(";1.5%-1.5;","%").empty());
968+
ASSERT(tokenValues(";1.5%-10.5;","%").empty());
969+
ASSERT(tokenValues(";1/-1;","/").empty());
970+
ASSERT(tokenValues(";1/-10;","/").empty());
971+
ASSERT(tokenValues(";1.5/-1;","/").empty());
972+
ASSERT(tokenValues(";1.5/-10;","/").empty());
973+
ASSERT(tokenValues(";1/-1.5;","/").empty());
974+
ASSERT(tokenValues(";1/-10.5;","/").empty());
975+
ASSERT(tokenValues(";1.5/-1.5;","/").empty());
976+
ASSERT(tokenValues(";1.5/-10.5;","/").empty());
977+
ASSERT(tokenValues(";1/0;","/").empty());
978+
ASSERT(tokenValues(";1/0;","/").empty());
979+
ASSERT(tokenValues(";1.5/0;","/").empty());
980+
ASSERT(tokenValues(";1.5/0;","/").empty());
961981
ASSERT(tokenValues(";((-1) * 9223372036854775807LL - 1) / (-1);", "/").empty()); // #12109
962-
ASSERT_EQUALS(tokenValues(";((-1) * 9223372036854775807LL - 1) % (-1);", "%").size(), 1);
982+
ASSERT_EQUALS(tokenValues(";((-1) * 9223372036854775807LL - 1) % (-1);", "%").size(), 1); // #12109
963983

964984
code = "float f(const uint16_t& value) {\n"
965985
" const uint16_t uVal = value; \n"

0 commit comments

Comments
 (0)