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