Am 19.12.2013 21:46, schrieb Andreas Tobler: > On 19.12.13 18:00, Stefan Esser wrote: > >> I'd replace the two occurances of LLONG_MAX with INT64_MAX and add the >> missing empty line: >> >> static __inline sbintime_t >> timer2sbintime(intptr_t data) >> { >> >> if (data > INT64_MAX / SBT_1MS) >> return INT64_MAX; >> return (SBT_1MS * data); >> } >> >> If you can show evidence that a limit of INT64_MAX/2 is more appropriate >> (2^30 seconds or 34 years), the limit could be of course be reduced to >> that value. >> >> I could not find any code that would not tolerate INT64_MAX, though ... > > Aehm, what about 32-bit systems where intptr_t == __int32_t? > > cc1: warnings being treated as errors > /export/devel/fbsd/src/sys/kern/kern_event.c: In function 'timer2sbintime': > /export/devel/fbsd/src/sys/kern/kern_event.c:529: warning: comparison is > always false due to limited range of data type
You are right, this needs to be fixed, too :( I see two possibilities: 1) Conditional compilation: There is no need to check an upper bound on ILP32 architectures. INT32_MAX seconds can be expressed as sbintime. Architectures where intptr_t has at least 48 bits will support the maximum time that can be expressed in sbintime and the comparison can be left as is for them. 2) Calculate the upper bound in such a way, that it is guaranteed to be lower than the highest value that can be expressed as intptr_t. This value is (INT32_MAX - 1) on 32 bit architectures, while it remains (INT64_MAX / SBT_1MS) on 64 bit architectures. I'm afraid that the warning emitted for 32 bit architectures will cause tinderbox build failures, but I haven't seen a failure message, yet. Should I back-out the commit that added the range check? As long as -fno-strict-overflow is not put back into CFLAGS, the test is not strictly required (only for the extremely unlikely case that a number > 1000 * MAX32_INT results in an extremely short remainder after multiplication with SBT_1MS modulo 32 ...). Regards, STefan NB: I should have known better and should have asked for a review of this change before it wa committed. Sorry for the inconvenience caused :( _______________________________________________ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"