[tz] Dubious coding in tzcode 2019b
eggert at cs.ucla.edu
Fri Jul 19 16:33:17 UTC 2019
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:
# include <stdbool.h>
# define true 1
# define false 0
# define bool int
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. 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
>> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 839 bytes
Desc: not available
More information about the tz