[tz] suggestions for potential code improvements?

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().
Zdump.c: yeartot()
Zic.c:	dolink(),itsdir(),inrule(),inzone(),inzcont().

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.

Regards,
Kees

-----Original Message-----
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?

Hello Paul,

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).

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.