On LP32 machines, the following code is dead: dayoff is long min_time is zic_t -> int_fast64_t if (dayoff < min_time / SECSPERDAY) { error(_("time too small")); return; } if (dayoff > max_time / SECSPERDAY) { error(_("time too large")); return; } The vax gcc-4.1 detects this; later versions of gcc don't. But all of them will warn, if you change min_time and max_time from const zic_t to #define's. What's the correct fix here? http://releng.netbsd.org/builds/HEAD/201303041020Z/vax.build.failed christos
I suggest changing change the compiler options to suppress the warnings, or at least to make the warnings not cause compilation to fail. There's nothing invalid about a comparison that happens to always be true on the VAX. Comparisons like that are normal and should be expected in code that tries to be portable among a wide variety of hardware platforms.
On Mar 5, 6:07pm, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: [tz] dead code in zic | I suggest changing change the compiler options | to suppress the warnings, or at least to make the warnings | not cause compilation to fail. There's nothing invalid | about a comparison that happens to always be true on the VAX. | Comparisons like that are normal and should be expected in | code that tries to be portable among a wide variety of | hardware platforms. Yes, and ILP32 is a portable and popular platform (still). This is not VAX specific. As I mentioned before if you change the const variable declaration to a #define, you'll get the warning in all gcc 32 bit platforms (or if you use a gcc circa 4.1 which had code to do the constant folding more aggressively). In simple terms, if the code is a no-op on all 32 bit platforms it should probably be improved! I know how to suppress warnings (with casts for example), and how to not compile with -Werror. What I am asking here, is if dayoff is mistyped (long instead of zic_t) and 'long oadd(long, long)' should be 'zic_t oadd(zic_t, long)'. This is yet another case where the code mis-uses variable sized types (long which is different on ILP32 and LP64) and could be improved. christos
...if the code is a no-op on all 32 bit platforms it should probably be improved!
While the code in question can surely be improved, it wasn't/isn't a no-op on systems where both time_t's and long's were/are 32-bit entities.
This is yet another case where the code mis-uses variable sized types (long which is different on ILP32 and LP64) and could be improved.
The code in question dates back to when "long" was the longest integer type available with all compilers ("long long" was not universal then). @dashdashado On Tue, Mar 5, 2013 at 9:47 PM, Christos Zoulas <christos@zoulas.com> wrote:
On Mar 5, 6:07pm, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: [tz] dead code in zic
| I suggest changing change the compiler options | to suppress the warnings, or at least to make the warnings | not cause compilation to fail. There's nothing invalid | about a comparison that happens to always be true on the VAX. | Comparisons like that are normal and should be expected in | code that tries to be portable among a wide variety of | hardware platforms.
Yes, and ILP32 is a portable and popular platform (still). This is not VAX specific. As I mentioned before if you change the const variable declaration to a #define, you'll get the warning in all gcc 32 bit platforms (or if you use a gcc circa 4.1 which had code to do the constant folding more aggressively). In simple terms, if the code is a no-op on all 32 bit platforms it should probably be improved!
I know how to suppress warnings (with casts for example), and how to not compile with -Werror. What I am asking here, is if dayoff is mistyped (long instead of zic_t) and 'long oadd(long, long)' should be 'zic_t oadd(zic_t, long)'.
This is yet another case where the code mis-uses variable sized types (long which is different on ILP32 and LP64) and could be improved.
christos
On Mar 5, 10:05pm, arthurdavidolson@gmail.com (Arthur David Olson) wrote: -- Subject: Re: [tz] dead code in zic | While the code in question can surely be improved, it wasn't/isn't a no-op | on systems where both time_t's and long's were/are 32-bit entities. It might not have been when zic_t was a time_t (if it ever was), but it is a no-op now because zic_t is int_fast64_t. If time_t is 32 or 64 is not relevant to the particular comparison. | The code in question dates back to when "long" was the longest integer type | available with all compilers ("long long" was not universal then). Yes, but things have changed and the code can be made more portable and work better across a wider set of platforms. christos
Thanks for the correction; I was remembering the bad old days when min_time was a time_t. So...in the present day, the "dayoff" code is, as you noted, a no-op on systems where longs are 32 bits. It is still needed on systems where longs are 64 bits. And since "sizeof" can't be used in "#if" directives, conditionalizing (to get rid of the dead code warning) isn't as easy as we might like. @dashdashado On Tue, Mar 5, 2013 at 10:11 PM, Christos Zoulas <christos@zoulas.com>wrote:
On Mar 5, 10:05pm, arthurdavidolson@gmail.com (Arthur David Olson) wrote: -- Subject: Re: [tz] dead code in zic
| While the code in question can surely be improved, it wasn't/isn't a no-op | on systems where both time_t's and long's were/are 32-bit entities.
It might not have been when zic_t was a time_t (if it ever was), but it is a no-op now because zic_t is int_fast64_t. If time_t is 32 or 64 is not relevant to the particular comparison.
| The code in question dates back to when "long" was the longest integer type | available with all compilers ("long long" was not universal then).
Yes, but things have changed and the code can be made more portable and work better across a wider set of platforms.
christos
On Tue, 05 Mar 2013, Arthur David Olson wrote:
The code in question dates back to when "long" was the longest integer type available with all compilers ("long long" was not universal then).
Today, intmax_t is the longest integer type. Perhaps most of the longs in zic.t should be changed to intmax_t. --apb (Alan Barrett)
On Tue, Mar 5, 2013 at 11:13 PM, Alan Barrett <apb@cequrux.com> wrote:
On Tue, 05 Mar 2013, Arthur David Olson wrote:
The code in question dates back to when "long" was the longest integer type available with all compilers ("long long" was not universal then).
Today, intmax_t is the longest integer type. Perhaps most of the longs in zic.t should be changed to intmax_t.
Does Microsoft's MSVC (which is a C89 compiler) have support for intmax_t (and <stdint.h> or <inttypes.h>)? Does the TZ code have to work with Microsoft compilers? Maintaining compatibility with MSVC in C (as opposed to C++) is a serious impediment to good engineering on occasion. -- Jonathan Leffler <jonathan.leffler@gmail.com> #include <disclaimer.h> Guardian of DBD::Informix - v2013.0118 - http://dbi.perl.org "Blessed are we who can laugh at ourselves, for we shall never cease to be amused."
On Mar 6, 7:06am, jonathan.leffler@gmail.com (Jonathan Leffler) wrote: -- Subject: Re: [tz] dead code in zic | Does Microsoft's MSVC (which is a C89 compiler) have support for intmax_t | (and <stdint.h> or <inttypes.h>)? Does the TZ code have to work with | Microsoft compilers? You are claiming that MSVC has int_fast64_t and does not have intmax_t? [I think it lacking both] | Maintaining compatibility with MSVC in C (as opposed to C++) is a serious | impediment to good engineering on occasion. Looks to me like this train has already left the station. christos
On 03/06/2013 07:42 AM, Christos Zoulas wrote:
You are claiming that MSVC has int_fast64_t and does not have intmax_t? [I think it lacking both]
It shouldn't matter. If we change zic.c to use intmax_t, we'll also change private.h to define intmax_t on older compilers that lack it, just as we already do with int_fast64_t.
On Wed, Mar 6, 2013, at 10:06, Jonathan Leffler wrote: On Tue, Mar 5, 2013 at 11:13 PM, Alan Barrett <[1]apb@cequrux.com> wrote: On Tue, 05 Mar 2013, Arthur David Olson wrote: The code in question dates back to when "long" was the longest integer type available with all compilers ("long long" was not universal then). Today, intmax_t is the longest integer type. Perhaps most of the longs in zic.t should be changed to intmax_t. Does Microsoft's MSVC (which is a C89 compiler) have support for intmax_t (and <stdint.h> or <inttypes.h>)? Does the TZ code have to work with Microsoft compilers? The latest version supports <stdint.h>, but NOT inttypes.h or printf("%jd" [etc]). I am unsure whether it supports long long (the headers are still full of __int64), but it does support %lld. I don't know what version these features were added in. References 1. mailto:apb@cequrux.com
On 2013/03/06 04:00 PM, random832@fastmail.us wrote:
On Wed, Mar 6, 2013, at 10:06, Jonathan Leffler wrote:
On Tue, Mar 5, 2013 at 11:13 PM, Alan Barrett <apb@cequrux.com <mailto:apb@cequrux.com>> wrote:
On Tue, 05 Mar 2013, Arthur David Olson wrote:
The code in question dates back to when "long" was the longest integer type available with all compilers ("long long" was not universal then).
Today, intmax_t is the longest integer type. Perhaps most of the longs in zic.t should be changed to intmax_t.
Does Microsoft's MSVC (which is a C89 compiler) have support for intmax_t (and <stdint.h> or <inttypes.h>)? Does the TZ code have to work with Microsoft compilers?
The latest version supports <stdint.h>, but NOT inttypes.h or printf("%jd" [etc]). I am unsure whether it supports long long (the headers are still full of __int64), but it does support %lld.
I don't know what version these features were added in.
<stdint.h> was added in Visual Studio 2010, but __int64 has been around since way before then (since at least Visual Studio 6.0 (VC98), probably before). Of course, porting anything back to VC98 is a complete pain in the posterior! -- -=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@mev.co.uk> )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
On 03/05/2013 06:47 PM, Christos Zoulas wrote:
As I mentioned before if you change the const variable declaration to a #define, you'll get the warning in all gcc 32 bit platforms (or if you use a gcc circa 4.1 which had code to do the constant folding more aggressively).
Yes, older GCCs are particularly bad about that; if you ask them to warn, they issue all sorts of bogus warnings. So it's better to not ask them to warn. Even newer GCCs mess up sometimes; and even then it's often better to ignore them.
if the code is a no-op on all 32 bit platforms it should probably be improved!
So far, all the suggestions for improvement would complicate the code, which will make it harder for humans to understand. Often in such cases it's better to keep the code simple, and to ignore or disable the bogus warnings.
What I am asking here, is if dayoff is mistyped (long instead of zic_t) and 'long oadd(long, long)' should be 'zic_t oadd(zic_t, long)'.
I'd go further. I expect the zic.c code would benefit by a uniform substitution of int_fast64_t for long -- but one would have to do considerably more than just a simple string substitution.
On Mar 5, 9:18pm, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: [tz] dead code in zic | I'd go further. I expect the zic.c code would benefit | by a uniform substitution of int_fast64_t for long -- but one would | have to do considerably more than just a simple string substitution. Thanks for the hint! I replaced: zic_t -> intmax_t long -> zic_t %ld -> %jd changed (not necessary but cleaner): static const zic_t min_time = INTMAX_MIN; static const zic_t max_time = INTMAX_MAX; and removed unused: #define OFFSET_STRLEN_MAXIMUM (7 + INT_STRLEN_MAXIMUM(long)) #define RULE_STRLEN_MAXIMUM 8 /* "Mdd.dd.d" */ Tested on both 32 and 64 bit hosts, same zone files produced by the old and the new zic. No compiler warnings. Best, christos
On Wed, Mar 6, 2013, at 13:25, Christos Zoulas wrote:
%ld -> %jd
%jd will not work on MSVC - of course, that's not actually the only problem stopping it from working on MSVC, or windows generally, at the moment. Is there any interest in making it work? Someone mentioned it earlier in the thread. -- Random832
participants (7)
-
Alan Barrett -
Arthur David Olson -
christos＠zoulas.com -
Ian Abbott -
Jonathan Leffler -
Paul Eggert -
random832＠fastmail.us