[tz] suggestions for potential code improvements?

[Kees Dekker]

Hi Paul,

I tried to compile the code (Visual Studio 2013 as first try, the other platforms will follow later), but I found some (minor) things:

1. The implementation of difftime() is specified with const arguments, while none of the referred places use these const qualifiers (e.g. private.h:368).
2. The code in difftime.c:29, 37 and 50 (a subtraction of two doubles) results in a: conversion from 'const time_t' to 'double', possible loss of data warning. Which is probably correct, as a time_t (64-bit integer) may not fit in a double (which is 2^52 according to IEEE 754). This compiler warning is not raised with gcc 4.8.3 on SUSE SLES 12. May be Visual Studio is somewhat pedantic in this case, but a cast will solve the issue.

Const mismatches:
3. There are (static) prototype mismatches between strftime.c:99 and the implementation of _fmt (arguments 2 and 4 have additional const in definition)
4.  idem for strftime.c:98 and the implementation of _conv() at line 572 (arguments 1, 2, 3 and 4 have additional const qualifiers)
5.  idem for strftime.c:97 and the implementation of _add() at line 580 (arguments 3 and 6 are different from declaration)
I don't really prefer one or other, but usually a compiler may optimize const qualified code somewhat better. 

If you like, I will also check the other files, but that will take additional time.

Regards,
Kees

-----Original Message-----
From: Kees Dekker 
Sent: Friday, July 24, 2015 10:53
To: 'Paul Eggert'; Time Zone Mailing List
Subject: RE: [tz] suggestions for potential code improvements?

Hi Paul,

Many thanks for your reply. The patch looks well. Will this patch become part of a successor version of the code?

I checked with my colleague (who checked the code in the past) and he told that some compiler may complain about assigning a 3-character string literal (which is actually 4 bytes) to an array of 3-byte strings, but I was not (yet) able to confirm this. My remark was just based on reading (only) the code.

Note that we use the tz code on a wide range of platforms (Windows/Visual Studio, AIX/XL C/C++, HPUX/aCC, Linux/gcc, Solaris/Studio Compiler). I will try to find a platform that complies about non-initialized variables. This will take some time for me before I have tested all platforms.

Regards,
Kees

-----Original Message-----
From: Paul Eggert [mailto:eggert at cs.ucla.edu] 
Sent: Friday, July 24, 2015 02:09
To: Kees Dekker; Time Zone Mailing List
Subject: Re: [tz] suggestions for potential code improvements?

On 07/23/2015 07:50 AM, Kees Dekker wrote:
>
> 1.Why do you not always initialize the variables that are now passed 
> to the INITIALIZE macro (see date.c/localtime.c/zic.c/private.h)? It 
> has (almost) no impact on performance and prevents strict compilers to 
> complain about (potentially) non-initialized variables.
>

Thanks for the careful reading of the code.  Although INITIALIZE does 
improve performance very slightly, it is not primarily about 
performance.  Mainly, it documents initialization that exists only to 
pacify compilers like GCC that would otherwise complain.  If the code 
always initialized variables even when not needed, the code would become 
more confusing to human readers, as we'd have to puzzle out why the 
initialization is present even though it is not used.

If this is a problem in your environment, you can compile with -Dlint.  
I often build this way:

make CFLAGS='$(GCC_DEBUG_FLAGS)'

as this provides -Dlint automatically.

> I don’t know whether all sprintf() implementations for all operating 
> systems respect the width/size specifier and allow non-0 terminated 
> string.
>

All sprintf implementations work that way.  This has been required by 
the C standard since C89 and is true for all C libraries in widespread 
use today.

I tried rewriting asctime.c along the lines that you suggested (see the 
attached patch).  On my platform (Fedora 21 x86-64, GCC 5.1.0) this made 
the zdump executable a tiny bit larger (7 bytes, for a new total of 
33077 bytes).  The wday_name and mon_name arrays are not likely to used 
elsewhere accidentally, as they're private to asctime_r and do not 
escape.  Although it's not a big deal either way, it should be OK to 
leave this code alone.