[tz] suggestions for potential code improvements?
Paul Eggert
eggert at cs.ucla.edu
Tue Jul 28 18:31:50 UTC 2015
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/attachment.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/attachment-0001.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/attachment-0002.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/attachment-0003.patch>
More information about the tz
mailing list