[tz] suggestions for potential code improvements?
Kees Dekker
Kees.Dekker at infor.com
Tue Jul 28 13:28:45 UTC 2015
Hello Paul,
Regarding date.c:
The patch in date.c is very *NIX centric. Windows does not have utmp.h (is not POSIX either), so the reset() function should stay behind #ifdef OLD_TIME.
Also the reset prototype is only needed with both OLD_TIME and NEW_TIME are defined.
My proposal is to put an #ifndef _WIN32 around #include "utmp.h" and add an additional:
#ifdef OLD_TIME
static void reset(time_t, int);
#else
#define reset(t,i)
#endif
Sorry, Windows is odd in much regard, and we need to get this code on Windows too...
I was a little bit surprised that no _WIN32 defines exist in your code...
The _WIN32 macro is a pre-defined macro. For its meaning see: https://msdn.microsoft.com/en-us/library/b0084kay.aspx.
Regards,
Kees
-----Original Message-----
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
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.
More information about the tz
mailing list