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 sort. 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 [-Werror,-Wliteral-conversion] register time_t half_second = 0.5; ~~~~~~~~~~~ ^~~ 1 error generated. Thanks, -- Andy Heninger
Looks as if the change below is what's needed; it is modeled on tzcode2013c. (The 0.5 is only relevant on systems where time_t is some variety of floating point; there may not be any such systems in the wild; the 0.5 should be optimized away by quality compilers on systems where time_t is some variety of integer.) --ado *** tzcode2013d/localtime.c 2013-05-28 00:26:18.000000000 -0400 --- tzcode2013e/localtime.c 2013-07-08 16:36:02.036867600 -0400 *************** *** 1514,1522 **** } { register int_fast32_t seconds; - register time_t half_second = 0.5; ! seconds = tdays * SECSPERDAY + half_second; tdays = seconds / SECSPERDAY; rem += seconds - tdays * SECSPERDAY; } --- 1514,1521 ---- } { register int_fast32_t seconds; ! seconds = tdays * SECSPERDAY + 0.5; tdays = seconds / SECSPERDAY; rem += seconds - tdays * SECSPERDAY; } On Mon, Jul 8, 2013 at 4:09 PM, Andy Heninger <aheninger@google.com> 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 sort.
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 [-Werror,-Wliteral-conversion] register time_t half_second = 0.5; ~~~~~~~~~~~ ^~~ 1 error generated.
Thanks, -- Andy Heninger
On Mon, 08 Jul 2013, Arthur David Olson wrote: [localtime.c]
--- 1514,1521 ---- } { register int_fast32_t seconds;
! seconds = tdays * SECSPERDAY + 0.5; tdays = seconds / SECSPERDAY; rem += seconds - tdays * SECSPERDAY; }
If time_t is an integer type, then that will multiply tdays * SECSPERDAY using time_t arithmetic, convert to double to add 0.5, then convert to int_fast32_t for the assignment. The conversion to and from double may lose precision, if double has less than 32 bits of mantissa precision (but that's unlikely). Perhaps this will work without either a compiler warning or potential loss of precision: seconds = tdays * SECSPERDAY + (time_t)0.5; --apb (Alan Barrett)
On Jul 8, 2013, at 5:19 PM, Alan Barrett wrote:
On Mon, 08 Jul 2013, Arthur David Olson wrote: [localtime.c]
--- 1514,1521 ---- } { register int_fast32_t seconds;
! seconds = tdays * SECSPERDAY + 0.5; tdays = seconds / SECSPERDAY; rem += seconds - tdays * SECSPERDAY; }
If time_t is an integer type, then that will multiply tdays * SECSPERDAY using time_t arithmetic, convert to double to add 0.5, then convert to int_fast32_t for the assignment. The conversion to and from double may lose precision, if double has less than 32 bits of mantissa precision (but that's unlikely).
Perhaps this will work without either a compiler warning or potential loss of precision:
seconds = tdays * SECSPERDAY + (time_t)0.5;
--apb (Alan Barrett)
I rather doubt it. Casting 0.5 to an integer type (as time_t is) ends up adding 0 or 1, but not 0.5. How about (tdays * SECSPERDAY * 2 + 1) / 2; ? paul
On Mon, 08 Jul 2013, Paul_Koning@Dell.com wrote:
Perhaps this will work without either a compiler warning or potential loss of precision:
seconds = tdays * SECSPERDAY + (time_t)0.5;
--apb (Alan Barrett)
I rather doubt it. Casting 0.5 to an integer type (as time_t is) ends up adding 0 or 1, but not 0.5.
The whole point of adding 0.5 before converting from time_t to int_fast32_t is to round up if time_t happens to be a floating point type, and to do nothing if time_t happens to be an integral type. You can't assume that time_t is an integral type.
How about (tdays * SECSPERDAY * 2 + 1) / 2; ?
Multiplying by 2 might overflow the available range. (I have not checked whether there are reasons why it would always be safe.) --apb (Alan Barrett)
Whatever the code ends up being, it would be helpful if it produced no compiler warnings with the warning level cranked up to the maximum, on either gcc or clang. -- Andy On Mon, Jul 8, 2013 at 2:49 PM, Alan Barrett <apb@cequrux.com> wrote:
On Mon, 08 Jul 2013, Paul_Koning@Dell.com wrote:
Perhaps this will work without either a compiler warning or potential
loss of precision:
seconds = tdays * SECSPERDAY + (time_t)0.5;
--apb (Alan Barrett)
I rather doubt it. Casting 0.5 to an integer type (as time_t is) ends up adding 0 or 1, but not 0.5.
The whole point of adding 0.5 before converting from time_t to int_fast32_t is to round up if time_t happens to be a floating point type, and to do nothing if time_t happens to be an integral type. You can't assume that time_t is an integral type.
How about (tdays * SECSPERDAY * 2 + 1) / 2; ?
Multiplying by 2 might overflow the available range. (I have not checked whether there are reasons why it would always be safe.) --apb (Alan Barrett)
Conditionalizing on "if (0.5 == (time_t) 0.5)" is a possibility (albeit ugly). --ado On Mon, Jul 8, 2013 at 6:00 PM, Andy Heninger <aheninger@google.com> wrote:
Whatever the code ends up being, it would be helpful if it produced no compiler warnings with the warning level cranked up to the maximum, on either gcc or clang.
-- Andy
On Mon, Jul 8, 2013 at 2:49 PM, Alan Barrett <apb@cequrux.com> wrote:
On Mon, 08 Jul 2013, Paul_Koning@Dell.com wrote:
Perhaps this will work without either a compiler warning or potential
loss of precision:
seconds = tdays * SECSPERDAY + (time_t)0.5;
--apb (Alan Barrett)
I rather doubt it. Casting 0.5 to an integer type (as time_t is) ends up adding 0 or 1, but not 0.5.
The whole point of adding 0.5 before converting from time_t to int_fast32_t is to round up if time_t happens to be a floating point type, and to do nothing if time_t happens to be an integral type. You can't assume that time_t is an integral type.
How about (tdays * SECSPERDAY * 2 + 1) / 2; ?
Multiplying by 2 might overflow the available range. (I have not checked whether there are reasons why it would always be safe.) --apb (Alan Barrett)
On 07/08/2013 03:00 PM, Andy Heninger wrote:
Whatever the code ends up being, it would be helpful if it produced no compiler warnings with the warning level cranked up to the maximum, on either gcc or clang.
That goal is too ambitious, as there are too many diagnostics if one enables them all, and too many of them are false alarms. The clang warning is a false alarm, as the code is portable and correct as-is, but I suppose it's worth working around the problem if it's easy -- more because the code is confusing to human readers than because clang is barfing on it. Arthur's first workaround makes the code slower and (as Alan mentioned) might introduce rounding errors in theory. As Alan also mentioned, Paul's idea of multiplying by 2 might overflow. Arthur's second workaround of adding a cast might work, but I prefer to avoid casts when possible, as they're too powerful -- it's too easy to convert a pointer to an integer by mistake, for example. So, how about this idea instead? It adds a function to do the conversion, with no casts. This lets us add commentary explaining the situation, and it pacifies clang for me.
From ba39973190ea28aa07b962a59e8bf73d4f54ef7a Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Mon, 8 Jul 2013 15:51:18 -0700 Subject: [PATCH] Rework to avoid bogus warning from clang -Wliteral-conversion.
Reported by Andy Heninger in <http://mm.icann.org/pipermail/tz/2013-July/019446.html>. * localtime.c (double_to_time): New function. (timesub): Use it. --- localtime.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/localtime.c b/localtime.c index 7db8ceb..d83b8c1 100644 --- a/localtime.c +++ b/localtime.c @@ -1434,6 +1434,18 @@ offtime(const time_t *const timep, const int_fast32_t offset) #endif /* defined STD_INSPIRED */ /* +** Convert T to time_t, truncating toward zero if time_t is integral. +** On most platforms, double_to_time(0.5) returns 0; the exceptions are +** the rare platforms where time_t is floating. +*/ + +static time_t +double_to_time(double t) +{ + return t; +} + +/* ** Return the number of leap years through the end of the given year ** where, to make the math easy, the answer for year zero is defined as zero. */ @@ -1514,7 +1526,7 @@ timesub(const time_t *const timep, const int_fast32_t offset, } { register int_fast32_t seconds; - register time_t half_second = 0.5; + register time_t half_second = double_to_time(0.5); seconds = tdays * SECSPERDAY + half_second; tdays = seconds / SECSPERDAY; -- 1.8.1.2
On 2013-07-08 22:19, Alan Barrett wrote:
On Mon, 08 Jul 2013, Arthur David Olson wrote: [localtime.c]
--- 1514,1521 ---- } { register int_fast32_t seconds;
! seconds = tdays * SECSPERDAY + 0.5; tdays = seconds / SECSPERDAY; rem += seconds - tdays * SECSPERDAY; }
If time_t is an integer type, then that will multiply tdays * SECSPERDAY using time_t arithmetic, convert to double to add 0.5, then convert to int_fast32_t for the assignment. The conversion to and from double may lose precision, if double has less than 32 bits of mantissa precision (but that's unlikely).
Perhaps this will work without either a compiler warning or potential loss of precision:
seconds = tdays * SECSPERDAY + (time_t)0.5;
For the record, that works fine in clang with no all warnings on, and the assembler output contains no floating point conversions even with optimization turned off. With optimization off, the generated code is also slightly smaller than my version that retained the 'half_second' variable with its initial value cast to type 'type_t'. -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On 2013-07-10 13:03, Ian Abbott wrote:
On 2013-07-08 22:19, Alan Barrett wrote:
Perhaps this will work without either a compiler warning or potential loss of precision:
seconds = tdays * SECSPERDAY + (time_t)0.5;
For the record, that works fine in clang with no all warnings on, and
That should be "no warnings on".
the assembler output contains no floating point conversions even with optimization turned off. With optimization off, the generated code is also slightly smaller than my version that retained the 'half_second' variable with its initial value cast to type 'type_t'.
-- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On 2013-07-10 13:17, Ian Abbott wrote:
On 2013-07-10 13:03, Ian Abbott wrote:
On 2013-07-08 22:19, Alan Barrett wrote:
Perhaps this will work without either a compiler warning or potential loss of precision:
seconds = tdays * SECSPERDAY + (time_t)0.5;
For the record, that works fine in clang with no all warnings on, and
That should be "no warnings on".
I mean "all warnings on" dammit!
the assembler output contains no floating point conversions even with optimization turned off. With optimization off, the generated code is also slightly smaller than my version that retained the 'half_second' variable with its initial value cast to type 'type_t'.
-- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
I noticed that a lot of source code changes were done in tzcode, for example many variables which were of type long have been changed to int_fast32_t. On my system, a x86_64 Linux, we get: typedef long int_fast32_t. So as expected, it comes out the same. long is 64 bit. But I wonder about the rationale behind the naming of this type: is it supposed to hold only 32-bit values? If it is supposed not to suffer from 32-bit overflow problems, i.e. meant to be wider than 32 bits, why is it named the way it is named? A general question: there is very little documentation for the source code in general. Would it not be appropriate for the maintainers of the code to include more internal documentation?
You should look up the details of the types defined in the C99 header <stdint.h>. It conditionally defines types like int32_t (it won't be defined if the machine has no 32-bit type — fairly unlikely these days, but there are/were 36-bit machines in times past), and unconditionally defines types such as int_least32_t and int_fast32_t. The type int_least32_t is an alternative name for a type that is at least 32 bits wide; int_fast32_t is an alternative name for the fastest type that is at least 32 bits wide. These types must be supported in C99 because long must be at least 32 bits wide. So, the names in use are carefully chosen from the C99 standard. On Sun, Jul 21, 2013 at 4:20 AM, Alois Treindl <alois@astro.ch> wrote:
I noticed that a lot of source code changes were done in tzcode, for example many variables which were of type long have been changed to int_fast32_t.
On my system, a x86_64 Linux, we get: typedef long int_fast32_t. So as expected, it comes out the same. long is 64 bit.
But I wonder about the rationale behind the naming of this type: is it supposed to hold only 32-bit values? If it is supposed not to suffer from 32-bit overflow problems, i.e. meant to be wider than 32 bits, why is it named the way it is named?
A general question: there is very little documentation for the source code in general. Would it not be appropriate for the maintainers of the code to include more internal documentation?
-- Jonathan Leffler <jonathan.leffler@gmail.com> #include <disclaimer.h> Guardian of DBD::Informix - v2013.0521 - http://dbi.perl.org "Blessed are we who can laugh at ourselves, for we shall never cease to be amused."
On Jul 21, 1:20pm, alois@astro.ch (Alois Treindl) wrote: -- Subject: Re: [tz] source code changes in tzcode2013d: int_:fast32_t | I noticed that a lot of source code changes were done in tzcode, for | example many variables which were of type long have been changed | to int_fast32_t. | | On my system, a x86_64 Linux, we get: typedef long int_fast32_t. | So as expected, it comes out the same. long is 64 bit. | | But I wonder about the rationale behind the naming of this type: is it | supposed to hold only 32-bit values? If it is supposed not to suffer | from 32-bit overflow problems, i.e. meant to be wider than 32 bits, why | is it named the way it is named? | | A general question: there is very little documentation for the source | code in general. Would it not be appropriate for the maintainers of the | code to include more internal documentation? The biggest problem with this change is that it is ABI changing in the following 3 functions: struct tm * offtime(const time_t * clock, long int offset); struct tm * offtime_r(const time_t * clock, long int offset, struct tm *ret); time_t timeoff(struct tm * tm, long int offset); In order not to change the ABI, I did not change them for NetBSD. The rest of the changes are internal, so they don't matter. And a cosmetic one, all the gcc attributes (not just some) should be __foo__ instead of foo, and the macros should be named consistenty. Finally my shameless plug, the NetBSD sources also contain the following changes: - multi-timezone support, thread safe code (_rz functions) - code to behave like glibc in the DST gap (not fail if enabled) - consistent errno setting on failure - man pages in mdoc not man format - modernization (s/sprintf/snprintf/, s/register//) christos
On 07/21/2013 04:20 AM, Alois Treindl wrote:
But I wonder about the rationale behind the naming of this type: is it supposed to hold only 32-bit values?
The tz code assigns only 32-bit values to it, yes. The type might be wider than that, on machines where making it wider is faster. Formerly, the type 'long' was used for values of this form, but I thought it better to be more precise -- more for documentation than for efficiency. On 07/21/2013 07:25 AM, Christos Zoulas wrote:
The biggest problem with this change is that it is ABI changing in the following 3 functions:
Thanks for reporting that. The changes to offtime and timeoff weren't intended. I didn't notice that they were extern functions (we don't have .h files, or I might have; we rely on the system's). The function gtime is also affected. (offtime_r isn't part of the tz distribution.) I've pushed the patch proposed below to the experimental version on github.
And a cosmetic one, all the gcc attributes (not just some) should be __foo__ instead of foo, and the macros should be named consistenty.
There's no need to put underscores around 'const', as it's a keyword, and programs and standard headers are not allowed to #define keywords. The macro _Noreturn is named specially because it stands for a C11 keyword, something the other macros don't do. At some point someone (maybe Arthur? or maybe me) should look at the NetBSD enhancements. Thanks for reminding us.
From 5ea7a94f6a57161575cac3a9698265c684d19e6c Mon Sep 17 00:00:00 2001 From: Paul Eggert <eggert@cs.ucla.edu> Date: Sun, 21 Jul 2013 08:24:45 -0700 Subject: [PATCH] Revert unintended change to ABI for extern functions.
* localtime.c (offtime, timeoff, gtime): Revert change to ABI, by going back to 'long' instead of 'int_fast32_t' for types accepted and returned by extern functions. Reported by Christos Zoulas in <http://mm.icann.org/pipermail/tz/2013-July/019488.html>. --- localtime.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/localtime.c b/localtime.c index d83b8c1..23bc635 100644 --- a/localtime.c +++ b/localtime.c @@ -1426,7 +1426,7 @@ gmtime_r(const time_t *const timep, struct tm *tmp) #ifdef STD_INSPIRED struct tm * -offtime(const time_t *const timep, const int_fast32_t offset) +offtime(const time_t *const timep, const long offset) { return gmtsub(timep, offset, &tm); } @@ -1997,7 +1997,7 @@ timegm(struct tm *const tmp) } time_t -timeoff(struct tm *const tmp, const int_fast32_t offset) +timeoff(struct tm *const tmp, const long offset) { if (tmp != NULL) tmp->tm_isdst = 0; @@ -2013,7 +2013,7 @@ timeoff(struct tm *const tmp, const int_fast32_t offset) ** previous versions of the CMUCS runtime library. */ -int_fast32_t +long gtime(struct tm *const tmp) { const time_t t = mktime(tmp); -- 1.8.1.2
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 sort.
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 [-Werror,-Wliteral-conversion] register time_t half_second = 0.5; ~~~~~~~~~~~ ^~~ 1 error generated.
Thanks, -- Andy Heninger
How about just changing it from an implicit conversion to an explicit one? register time_t half_second = (time_t)0.5; ? -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
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 sort.
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 [-Werror,-Wliteral-conversion] register time_t half_second = 0.5; ~~~~~~~~~~~ ^~~ 1 error generated.
Thanks, -- 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. -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
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 sort.
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 [-Werror,-Wliteral-conversion] register time_t half_second = 0.5; ~~~~~~~~~~~ ^~~ 1 error generated.
Thanks, -- 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! -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On 07/09/2013 07:11 AM, Ian Abbott wrote:
However, my version is faster (for non-floating-point time_t) if you leave out the optimization options
True, but for system builders who care about performance, it's safe to assume nowadays that optimization will be enabled and the compiler will inline the conversion function. And those who don't care about performance won't care a whit about the difference, except maybe to prefer the slower version because it makes things clearer during debugging. Come to think of it, several macros in the tz source code could be changed to functions: is_digit, _, isleap, isleap_sum, isascii, end, emalloc, erealloc, ecpyalloc, ecatalloc. Functions tend to be easier to debug than macros, and there's no real performance reason to prefer macros nowadays.
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 sort.
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 [-Werror,-Wliteral-conversion] register time_t half_second = 0.5; ~~~~~~~~~~~ ^~~ 1 error generated.
Thanks, -- 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. Martin -- Martin Burnicki Meinberg Funkuhren Bad Pyrmont Germany
On 07/09/13 07:34, Martin Burnicki wrote:
Making double_to_time() an inline function would be better, but I'm sure this is not portable enough.
Making double_to_time inline won't be better performance-wise, as any decent compiler nowadays will inline that function while optimizing, even if the function is not declared to be inline. With typical modern compilers there's little point to declaring static functions to be inline. Plain static is good enough; the compiler will inline the function if it makes sense. (extern inline is another story, but that's not the issue here.)
participants (10)
-
Alan Barrett -
Alois Treindl -
Andy Heninger -
Arthur David Olson -
christos＠zoulas.com -
Ian Abbott -
Jonathan Leffler -
Martin Burnicki -
Paul Eggert -
Paul_Koning＠Dell.com