[tz] suggestions for potential code improvements?
Kees.Dekker at infor.com
Tue Jul 28 13:45:10 UTC 2015
Sorry, to bother again:
Both date.c, zdump.c, zic.c contain a number of static functions where the declaration differs from the implementation. This applies to:
Date.c: wildinput(), oops(), display(), timeout(), iffy().
I also found that Visual Studio is the compiler that reports (line numbers corrected to match with your sources):
1>w:\logic\tz\zdump.c(970): error C4295: 'wday_name' : array is too small to include a terminating null character
1>w:\logic\tz\zdump.c(974): error C4295: 'mon_name' : array is too small to include a terminating null character
Which is solved by incrementing the '3' to '4' of both arrays.
Some other remarks of the previous email (like gmtime_r() does not exist on Windows, and regarding conversion errors from size_t to int) also exist in the previously mentioned 3 source files, but I first like to know your thoughts regarding the mail below before I will provide additional feedback.
From: Kees Dekker
Sent: Tuesday, July 28, 2015 14:31
To: 'Paul Eggert'
Cc: Time zone mailing list
Subject: RE: [tz] suggestions for potential code improvements?
Many thanks. When I now refer to line numbers, it is to your sources, patched with the patches supplied in previous email.
A minor note about difftime.c: 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. Personally I think a cast is ok here, but it is up to you to decide something else. I do not need a patch for this anymore (to solve all warnings I now have to add even more casts, compared to previous version :-)).
Notes for localtime.c:
- The functions typesequiv(), tzparse (), timesub(), increment_overflow(), normalize_overflow32() differ in const qualifiers between declarations and definitions.
- Windows does not have gmtime_r() (and none of the other time related xxx_r() functions), but the Windows gmtime() is already thread-safe. See also: https://msdn.microsoft.com/en-us/library/0z9czt0w.aspx and e.g. http://stackoverflow.com/questions/12044519/what-is-the-windows-equivalent-of-the-unix-function-gmtime-r.
- 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). Because stdlen is used to hold the return value of e.g. strlen(), its size_t type is good, but raises a C4276 warning (conversion from 'size_t' to 'int', possible loss of data). This warning is treated as error in our own code. The same happens on line 1186. Similar conversion errors are at lines 1197 and 1199 (because charcnt is an int, and stdlen and dstlen are of size_t type) and 1594 (idelta is an int), 1609 (second is an int, tdays a 64-bit time_t), 1616 (idays is an int, tdays not).
From: Paul Eggert [mailto:eggert at cs.ucla.edu]
Sent: Monday, July 27, 2015 21:13
To: Kees Dekker
Cc: Time zone mailing list
Subject: Re: [tz] suggestions for potential code improvements?
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
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
> 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
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.
More information about the tz