[tz] Dubious coding in tzcode 2019b

[Clive D.W. Feather]

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 at davros.org     | it will get its revenge.
Web: http://www.davros.org  |   - Henry Spencer
Mobile: +44 7973 377646