[tz] Dubious coding in tzcode 2019b
Clive D.W. Feather
clive at davros.org
Fri Jul 19 07:53:40 UTC 2019
Tom Lane said:
> Some compilers in the Postgres buildfarm are unhappy 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
(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
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
More information about the tz