[tz] localtime.c issue
martin.burnicki at meinberg.de
Tue Jul 9 14:34:05 UTC 2013
Ian Abbott wrote:
> On 2013-07-09 14:53, Ian Abbott wrote:
>> On 2013-07-09 11:06, Ian Abbott wrote:
>>> On 2013-07-08 21:09, Andy Heninger wrote:
>>>> This compilation error from the localtime.c in 2013d looks like an
>>>> actual problem. I think time_t is generally an integer type of some
>>>> Built using the clang compiler on Linux.
>>>> localtime.c:1517:34: error: implicit conversion from 'double' to
>>>> 'time_t' (aka 'long') changes value from 0.5 to 0
>>>> register time_t half_second = 0.5;
>>>> ~~~~~~~~~~~ ^~~
>>>> 1 error generated.
>>>> -- Andy Heninger
>>> How about just changing it from an implicit conversion to an
>>> explicit one?
>>> register time_t half_second = (time_t)0.5;
>> I've just tested that with
>> make CC=clang CFLAGS="-Werror -Wall"
>> It seems to work. But Paul has already fixed it with the double_to_time
>> function, so my fix is superfluous.
> However, my version is faster (for non-floating-point time_t) if you
> leave out the optimization options, like I did. Without optimization,
> the double_to_time function actually does get called!
And IMO your version is more straightforward. It works as expected if
time_t is a floating point type, and just uses 0 (as expected) in usual
cases where time_t is some kind of integer.
I'm sure most people who might look at the code don't even know there
could be a floating point implementation of time_t. I've never seen such
system, either. So a single line comment might help to explain what this
is for, e.g.:
/* used for rounding in case time_t is a floating point type */
Even though I also avoid using casts, I see no danger using it in a case
like this, and this is probabbly exactly what casts are for. This can be
evaluated at compile time and doesn't require a function call. Making
double_to_time() an inline function would be better, but I'm sure this
is not portable enough. So using a cast keeps the code simple, and the
execution time short.
More information about the tz