Dubious coding in tzcode 2019b
Some compilers in the Postgres buildfarm are unhappy[1] about this coding in 2019b's zic.c: register ptrdiff_t thistimei, thistimecnt, thistimelim; bool locut, hicut; ... thistimecnt = - locut - hicut; I think they have a point: at best this code is seriously opaque, and at worst it's unportable. I can't even figure out whether this is assuming that bool promotes to signed or unsigned int; and even if whichever choice that is supported by the C99 spec, what will happen in environments where "bool" does not mean the C99 type, but rather signed or unsigned char? (For my immediate purposes, that includes every released branch of Postgres :-(.) I think this would be a lot better if it were written as some if-tests that just consider locut and hicut as boolean values, without any assumptions about how bool converts to integer. If we must have such assumptions, please write them as explicit casts. While the compilers aren't whining about it, this line a bit further down seems like equally bad style to me: convert(locut + thistimecnt + hicut, tzh.tzh_timecnt); This also seems like it's making dubious assumptions about how signed and possibly-unsigned values will mix. regards, tom lane [1] https://www.postgresql.org/message-id/20190719035347.GJ1859@paquier.xyz
Tom Lane said:
Some compilers in the Postgres buildfarm are unhappy[1] about this coding in 2019b's zic.c:
register ptrdiff_t thistimei, thistimecnt, thistimelim; bool locut, hicut; ... thistimecnt = - locut - hicut;
Gagh. The spacing there makes it look like it's intended to be -(lo - hi) when of course it's (-lo) - hi.
I think they have a point: at best this code is seriously opaque, and at worst it's unportable. I can't even figure out whether this is assuming that bool promotes to signed or unsigned int; and even if whichever choice that is supported by the C99 spec,
In C99 bool always promotes to signed int.
what will happen in environments where "bool" does not mean the C99 type, but rather signed or unsigned char?
Both signed and unsigned char promote to signed int *EXCEPT* on implementations where sizeof(int) is 1, where they each promote to the correspondingly-signed int. (Yes, such implementations exist, though they require CHAR_BIT >= 16. We used one where I work until a couple of years ago.)
I think this would be a lot better if it were written as some if-tests that just consider locut and hicut as boolean values, without any assumptions about how bool converts to integer. If we must have such assumptions, please write them as explicit casts.
I'm assuming whoever wrote that didn't think of the unusual case, because that expression will end up as 0, INT_MAX-1, or INT_MAX-2. Which when converted to ptrdiff_t is either unchanged (if the maximum value of ptrdiff_t is big enough), or is an implementation-defined value, or an implementation-defined signal is raised. You could write it as a conditional expression: thistimecnt = locut && hicut ? -2 : locut || hicut ? -1 : 0; but I think I would write it as: thistimecnt = - (ptrdiff_t) (locut + hicut); because that shows the intent better.
While the compilers aren't whining about it, this line a bit further down seems like equally bad style to me:
convert(locut + thistimecnt + hicut, tzh.tzh_timecnt);
This also seems like it's making dubious assumptions about how signed and possibly-unsigned values will mix.
If locut and hicut haven't changed in the meantime, that first parameter is always zero! If they have, then the intent was a number in the range -2 to +2. Except for the unusual case, that should always come out as a ptrdiff_t. -- Clive D.W. Feather | If you lie to the compiler, Email: clive@davros.org | it will get its revenge. Web: http://www.davros.org | - Henry Spencer Mobile: +44 7973 377646
Clive D.W. Feather wrote:
I'm assuming whoever wrote that didn't think of the unusual case,
The unusual case cannot happen in any compiler conforming to C89 or later, because private.h does this: #if HAVE_STDBOOL_H # include <stdbool.h> #else # define true 1 # define false 0 # define bool int #endif and either way, bool is guaranteed to promote to int. The problem is independent of PostgreSQL's attempt to redefine bool, because zic.c does not include PostgresSQL's c.h file. The problem occurs because the Microsoft C compiler warns about negating a bool variable.[1] The Microsoft compiler generates correct code, so this style warning is harmless.
I think I would write it as:
thistimecnt = - (ptrdiff_t) (locut + hicut);
because that shows the intent better. I would rather avoid unnecessary casts, so the attached proposed patch does it the way you suggest except without the cast. This should make the source code a bit clearer, and maybe it will pacify the Microsoft compiler.
I'm not worried about the style issue of treating true and false as 1 and 0, as the idiom should be reasonably obvious to readers who know C. (Anybody who's been around long enough to remember C's predecessor BCPL where true was all-bits-1, should know by now that C does things differently. :-) So if the Microsoft compiler warns even with the patch, I'm somewhat inclined to suggest that people just ignore these bogus warnings, or use a compiler flag that shuts them off.
While the compilers aren't whining about it, this line a bit further down seems like equally bad style to me:
convert(locut + thistimecnt + hicut, tzh.tzh_timecnt);
This also seems like it's making dubious assumptions about how signed and possibly-unsigned values will mix.
We're OK here, as the values cannot possibly be unsigned as mentioned above.
If locut and hicut haven't changed in the meantime, that first parameter is always zero! If they have, then the intent was a number in the range -2 to +2. Except for the unusual case, that should always come out as a ptrdiff_t.
That first parameter can be nonzero when the "thistimecnt = - locut - hicut;" assignment is skipped. The parameter is always nonnegative. Reference [1] https://docs.microsoft.com/en-us/cpp/error-messages/compiler-warnings/compil...
On 19 Jul 2019, at 17:33, Paul Eggert <eggert@cs.ucla.edu> wrote:
I think I would write it as: thistimecnt = - (ptrdiff_t) (locut + hicut); because that shows the intent better. I would rather avoid unnecessary casts, so the attached proposed patch does it the way you suggest except without the cast. This should make the source code a bit clearer, and maybe it will pacify the Microsoft compiler.
I'm not worried about the style issue of treating true and false as 1 and 0, as the idiom should be reasonably obvious to readers who know C. (Anybody who's been around long enough to remember C's predecessor BCPL where true was all-bits-1, should know by now that C does things differently. :-) So if the Microsoft compiler warns even with the patch, I'm somewhat inclined to suggest that people just ignore these bogus warnings, or use a compiler flag that shuts them off.
You'll see "x = !!foo" quite often in the Linux kernel which converts any non-zero integer to "1". It certainly makes it crystal clear that x is 0 or 1. As an aside, C's lack of a strong boolean type has caused all kinds of problems down the years. jch
Paul Eggert said:
The unusual case cannot happen in any compiler conforming to C89 or later, because private.h does this:
#if HAVE_STDBOOL_H # include <stdbool.h> #else # define true 1 # define false 0 # define bool int #endif
Hmm, I would have: typedef int bool; to stop someone writing "unsigned bool". Also, that definition is going to cause problems if you have "bool q : 1" within a structure; you need it to be unsigned.
I think I would write it as:
thistimecnt = - (ptrdiff_t) (locut + hicut);
because that shows the intent better. I would rather avoid unnecessary casts, so the attached proposed patch does it the way you suggest except without the cast.
Your call; I think the cast helps in this case.
I'm not worried about the style issue of treating true and false as 1 and 0, as the idiom should be reasonably obvious to readers who know C.
Ah, my code either has: enum { false, true }; or: #define false (0 == 1) #define true (!true) or, when I'm in a whimiscal mood: #define true ('/'/'/') #define false ('-'-'-')
(Anybody who's been around long enough to remember C's predecessor BCPL where true was all-bits-1,
Oh no it wasn't. TRUE and FALSE were implementation-defined values. The only requirement was that the bitwise operators such as & and NEQV gave the correct results when applied to them. So, for example, it would be legitimate for TRUE to be 7 and FALSE to be 3, with logical mode testing the 4 bit. -- Clive D.W. Feather | If you lie to the compiler, Email: clive@davros.org | it will get its revenge. Web: http://www.davros.org | - Henry Spencer Mobile: +44 7973 377646
Paul Eggert <eggert@cs.ucla.edu> writes:
I would rather avoid unnecessary casts, so the attached proposed patch does it the way you suggest except without the cast. This should make the source code a bit clearer, and maybe it will pacify the Microsoft compiler.
I can confirm that this makes our buildfarm happier. Thanks for the fix! regards, tom lane
participants (4)
-
Clive D.W. Feather -
John Haxby -
Paul Eggert -
Tom Lane