[tz] Dubious coding in tzcode 2019b

[Paul Eggert]

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/compiler-warning-level-1-c4804
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Clarify-bool-calculation.patch
Type: text/x-patch
Size: 839 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20190719/1667e106/attachment.bin>