Closed esoule closed 10 years ago
I found out that that section of tests under "if (sizeof(int_least32_t) == 8)" is not compiled in on x86_64, by adding a non-existent extern variable and finding out that it is optimized away (no linker error).
The compiler warning is emitted, though.
diff --git a/tests.c b/tests.c
index da7683f..b41905d 100644
--- a/tests.c
+++ b/tests.c
@@ -107,12 +107,14 @@ START_TEST(test_minmea_scan_d)
ck_assert_int_eq(direction, -1);
ck_assert(minmea_scan("E,foo", "d", &direction) == true);
ck_assert_int_eq(direction, 1);
}
END_TEST
+extern int _does_not_exist;
+
START_TEST(test_minmea_scan_f)
{
struct minmea_float f;
ck_assert(minmea_scan("-", "f", &f) == false);
ck_assert(minmea_scan("10-", "f", &f) == false);
@@ -158,17 +160,17 @@ START_TEST(test_minmea_scan_f)
ck_assert_int_eq(f.scale, 100);
/* doesn't fit, bail out */
ck_assert(minmea_scan("2147483648", "f", &f) == false);
} else if (sizeof(int_least32_t) == 8) {
/* fits in 64 bits */
ck_assert(minmea_scan("9223372036854775807", "f", &f) == true);
- ck_assert_int_eq(f.value, 9223372036854775807);
+ ck_assert_int_eq(f.value + _does_not_exist, 9223372036854775807);
ck_assert_int_eq(f.scale, 1);
/* doesn't fit, truncate precision */
ck_assert(minmea_scan("9223372036854775.808", "f", &f) == true);
- ck_assert_int_eq(f.value, 922337203685477580);
+ ck_assert_int_eq(f.value + _does_not_exist, 922337203685477580);
ck_assert_int_eq(f.scale, 100);
/* doesn't fit, bail out */
ck_assert(minmea_scan("9223372036854775808", "f", &f) == false);
} else {
ck_abort_msg("your platform is esoteric. please fix this unit test.");
}
The first warning is harmless. I added some #pragma diagnostic
magic to prevent it from popping up (see df695c49).
As for the second one - I can't reproduce it, what compiler and platform are you using? I have to admit I'm not an expert on 64-bit; I wrote this library with microcontrollers in mind, so I'm targetting mostly 32-bit ARM. Maybe adding "LL" at the end of the large integer constant will help? Also, I don't really like the conditional testing, but as I said, 64-bit is mostly a nuisance for me.
I fixed this warning by replacing "x == y" comparisons with "fabs(x - y) <= epsilon" (and I set epsilon to zero for these test cases): f68eb7c9fd968bea0811bdda521e29092bbbcc01
Of course, I had to undo df695c4933cfe807c4889dce30a27800e2e2dc85 with 52c077c5acfe04e3fbf509e9790392ecd4bf0700 - because with f68eb7c9fd968bea0811bdda521e29092bbbcc01 it is no longer necessary. (I also found out that gcc 4.4.7 does not support #pragma GCC diagnostic push|pop
)
Edit: for floats, fabsf
should be used, not fabs
(40b3fdd80d995915066b04c95023c9593524a997). My bad.
Fixed in #9, closing.
Note that x==y
and fabs(x-y)<=0.0
are not equivalent, for x=y=Inf or -Inf... because Inf-Inf is NaN and NaN can't be compared. So even if some compiler maintainer had the idea to re-optimize fabs(x-y)<=0.0 into x==y he couldn't ;)
@nicolas-cellier-aka-nice It's okay in our use case (unit tests); one of the values in comparison is always constant and non-infinity. I wouldn't be terribly happy to see it in production code, though..
I trust you. Since this is one of the first hits when googling -Wfloat-equal, at least I will have warned against using such substitution as a generic workaround. If a compiler warning potentially lead to introduce more problems in code then it must be in knowledge base.
@nicolas-cellier-aka-nice Ah, I wasn't aware of such popularity. :) Yes, -Wfloat-equal is there for a reason. Outside unit tests, comparing floats for equality is just plain dangerous, and even in unit tests it needs good understanding of how it works. The values I used for unit tests have been carefully chosen to be exactly representable under IEEE754. This is not a properly most numbers have - especially with externally-supplied values.
When compiling with the CFLAGS below
I am getting these warnings:
The line "comparison is always false due to limited range of data type" is likely due to the fact that integer is 32-bit on x86_64 and number 9223372036854775807 does not fit into 32 bits.
The line above is under "if (sizeof(int_least32_t) == 8)" though