[tz] suggestions for potential code improvements?

[Paul Eggert]

Kees Dekker wrote:

> Visual Studio even complains if a time_t (variable or result of a arithmetic operation) is passed to a function (dminus()) that accepts a double.

Yes, let's not worry about that.  This sort of conversion is portable and 
standard-conforming C code, and inserting a cast would make the code harder to 
read and more error-prone.

> Notes for localtime.c:
> - The functions typesequiv(), tzparse (), timesub(), increment_overflow(), normalize_overflow32() differ in const qualifiers between declarations and definitions.

Thanks, fixed in the attached patch #1.

> - init_ttinfo() accepts as last argument an int, but e.g. stdlen (localtime.c:988) is a size_t and passed to init_ttinfo() (at line 1065).

That size_t value should never exceed INT_MAX, so there shouldn't be a problem 
here.  However, now that you mention it I see an unlikely possibility that 
stdlen would exceed INT_MAX.  Fixed in attached patch #2.

> Similar conversion errors are at lines 1197 and 1199 (because charcnt is an int, and stdlen and dstlen are of size_t type)

Patch #2 should also fix the actual bug here.  I'm not so much worried about the 
VS diagnostics, if they're false alarms -- basically, VS appears to be 
complaining about all implicit conversions and this generates false alarms in 
tzcode.  While we're in the neighborhood I found a minor bug in error-checking 
the newfangled angle-bracket abbreviations; fixed in attached patch#3.

> and 1594 (idelta is an int), 1609 (second is an int, tdays a 64-bit time_t), 1616 (idays is an int, tdays not).

Line 1594 ("idelta = tdelta;") is safe, as the previous three lines guarantee 
that INT_MIN <= tdelta && tdelta <= INT_MAX.  Similarly, line 1616 ("idays = 
tdays;") is safe, as tdays must be in the range 0..366 here (this is guaranteed 
by the previous while loop).  Although line 1609 is also safe I see that it can 
be removed; it dates back to when the code was intended to be portable to 
platforms where time_t is a floating-point type; this never really worked and 
was removed in 2013 and this code is now unnecessary.  Fixed in attached patch #4.

I've installed the attached proposed patches into the experimental version on 
github.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Pacify-Visual-Studio-2013-const-some-more.patch
Type: text/x-diff
Size: 2119 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20150728/92671156/0001-Pacify-Visual-Studio-2013-const-some-more.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Fix-int-overflow-with-very-long-string.patch
Type: text/x-diff
Size: 2415 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20150728/92671156/0002-Fix-int-overflow-with-very-long-string.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-localtime.c-tzparse-Fix-test-for-empty-std-abbr.patch
Type: text/x-diff
Size: 683 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20150728/92671156/0003-localtime.c-tzparse-Fix-test-for-empty-std-abbr.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Simplify-timesub-based-on-integer-time_t.patch
Type: text/x-diff
Size: 1121 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20150728/92671156/0004-Simplify-timesub-based-on-integer-time_t.patch>