[tz] suggestions for potential code improvements?
Paul Eggert
eggert at cs.ucla.edu
Mon Jul 27 19:13:08 UTC 2015
Thanks again for the careful reading of the code. Here are some thoughts about
your comments. I've applied the attached patches to the experimental version on
github.
Kees Dekker wrote:
> 1>w:\general\lib\tz\strftime.c(595): error C4028: formal parameter 6 different from declaration
Thanks, I missed that one. Fixed in the first attached patch.
> we have changed the code of the tzcode source code files in the past
OK. It would be easier if you could communicate to us via the source code that
we have, so that we can understand the line numbers. One good way is to use Git
and to send us patches. Something like the following shell command:
git clone https://github.com/eggert/tz.git
Then make the changes you like to the source code, and then run the shell
command 'git diff' and send us the output.
> there is already such cast at two places in this difftime() function
Then let's get rid of those casts too. I did that by installing the second
attached patch.
> I do not see any problem to resolve some warnings and add a cast in this case.
Introducing casts makes the code a bit harder to read and therefore a bit less
reliable on that account. Readability is more important than pacifying
misguided compilers (though we make an exception if recent GCC is misguided).
> passing flags withing visual studio is somewhat different
You can also prepend '#define lint 1' to your copy of private.h.
> our Linux systems (SLES11SP3, SLES12, RHAT6.6) has a too old gcc compiler.
That's OK. We can treat old GCC versions the same way we treat any other
compiler. That is, the idea is to use $(GCC_DEBUG_FLAGS) only with
recent-enough versions of GCC. I've documented this in the third attached patch.
> At least the two following errors are raised by Visual Studio 2014 (update 5):
>
> 2>w:\general\lib\tz\localtime.c(1182): error C4701: potentially uninitialized
local variable 'dstname' used
> 2>w:\general\lib\tz\localtime.c(931): error C4701: potentially uninitialized
local variable 'value' used
Both diagnostics are false alarms, as the local variables can never be used
uninitialized.
For 'dstname', evidently Visual Studio 2014u5 isn't tracking local variables as
well as GCC does. I don't see any easy workaround that wouldn't unnecessarily
clutter the code via needless calls to INITIALIZE or whatever.
For 'value', perhaps Visual Studio 2014u5 will be pacified by using an enum
instead. That would make the code a bit clearer anyway, so it's worth doing
regardless of whether Visual Studio complains. I installed the fourth attached
patch to do this.
> These errors are caused by the fact that we turn warning C4701 into an errors. These are actually warnings. But we turned these into errors, because potentially uninitialized code is very hard to fix one customers complain about (random) problems.
One way to fix your problem is to stop turning C4701 into errors, at least when
compiling localtime.c. No doubt there's a compiler switch to do that.
> the code in tzparse() contains code paths that do no set dstname
Yes, but if that happens dstname is not used later, so there's no bug there.
> the switch in transtime() does not have a default case
Perhaps this is fixed by using the enum as mentioned above.
> date.c: Contains two calls to lose(2), where I think that close(2) is intended
Thanks, that's a typo that I introduced last year. This code is used only on
platforms that have BSD but not SystemV interfaces for setting time. As the
SystemV version has been part of POSIX for quite some time, I imagine that the
code is dead now, so let's fix the typo (fifth attached patch) and then remove
this code (sixth attached patch).
> uint_fast64_t. Can you please add such type in private.h?
Sure, done in the seventh attached patch.
> my final goal is to check the (in our case) poor performance of opening/reading the zone files.
Have you looked into the functions tzalloc, localtime_rz, etc.? They solve the
performance problem in a different way. They're thread- and signal-safe, so I
expect they'd be better for users than the error-prone process of switching back
and forth among time zone settings.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Pacify-Visual-Studio-2013-const-again.patch
Type: text/x-diff
Size: 802 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20150727/bf26525a/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Avoid-unnecessary-casts-in-difftime.patch
Type: text/x-diff
Size: 2949 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20150727/bf26525a/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Makefile-GCC_DEBUG_FLAGS-Document-that-it-s-for-rece.patch
Type: text/x-diff
Size: 887 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20150727/bf26525a/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Be-clearer-about-r_type-s-values.patch
Type: text/x-diff
Size: 1439 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20150727/bf26525a/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Replace-lose-by-close-to-fix-date.c-typo.patch
Type: text/x-diff
Size: 705 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20150727/bf26525a/attachment-0004.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-date-n-is-now-an-undocumented-no-op.patch
Type: text/x-diff
Size: 10192 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20150727/bf26525a/attachment-0005.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-Port-uint_fast64_t-to-Visual-Studio.patch
Type: text/x-diff
Size: 1420 bytes
Desc: not available
URL: <http://mm.icann.org/pipermail/tz/attachments/20150727/bf26525a/attachment-0006.patch>
More information about the tz
mailing list