[tz] suggestions for potential code improvements?

[Kees Dekker]

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.