[tz] suggestions for potential code improvements?

Kees Dekker Kees.Dekker at infor.com
Mon Jul 27 14:53:11 UTC 2015


Hi,

1.  I was talking about a static function that haves its declaration and definition in one single file, but these two differ... That's IMO incorrect.
Note (for all files): we have changed the code of the tzcode source code files in the past (own header file), so that is why the line numbers differ (with a 2 or 4 line offset). I already used the latest source (2015e).

a. difftime.c: build well now after applying supplied patch as sent by Paul Eggert in a previous email.

b. The strftime.c file and other were adjusted by us in the past, so that is why the line numbers differ (with a 2 or 4 line offset). I already used the latest source (2015e).
However, even with the patch supplied, I get the following build errors with Visual Studio:

1>w:\general\lib\tz\strftime.c(595): error C4028: formal parameter 6 different from declaration

The declaration of _yconv() on line 593 (your code: line 589) differs from its declaration on line 100 (line 96) in your source. According how you changed the rest, I guess the second const for argument 6 of _yconv() function at line 589 (its definition) need to be removed.

2. difftime.c: My idea was indeed to change the code to return (double) (time1 - time0);

I agree with you that (in general) preventing casts is preferred if it is just used to make a compiler happy. Although much compiler warnings in a large code base is also confusing/a source of noise. Because there is already such cast at two places in this difftime() function, I think it is not a problem to add the casts in 3 additional places too. In my case, difftime() becomes:

double ATTRIBUTE_CONST
difftime(const time_t time1, const time_t time0)
{
	/*
	** If (sizeof (double) > sizeof (time_t)) simply convert and subtract
	** (assuming that the larger type has more precision).
	*/
	if (sizeof (double) > sizeof (time_t))
		return (double) time1 - (double) time0;
	if (!TYPE_SIGNED(time_t)) {
		/*
		** The difference of two unsigned values can't overflow
		** if the minuend is greater than or equal to the subtrahend.
		*/
		if (time1 >= time0)
			return  (double) (time1 - time0);
		else	
			return -(double) (time0 - time1);
	}
	/*
	** Handle cases where both time1 and time0 have the same sign
	** (meaning that their difference cannot overflow).
	*/
	if ((time1 < 0) == (time0 < 0))
		return (double)(time1 - time0);
	/*
	** time1 and time0 have opposite signs.
	** Punt if uintmax_t is too narrow.
	** This suffers from double rounding; attempt to lessen that
	** by using long double temporaries.
	*/
	if (sizeof (uintmax_t) < sizeof (time_t))
		return (long double) time1 - (long double) time0;
	/*
	** Stay calm...decent optimizers will eliminate the complexity below.
	*/
	if (time1 >= 0 /* && time0 < 0 */)
		return  (double) ((uintmax_t) time1 + (uintmax_t) (-1 - time0) + 1);
	else
		return -(double) ((uintmax_t) time0 + (uintmax_t) (-1 - time1) + 1);
}

I do not see any problem to resolve some warnings and add a cast in this case. It makes the build output cleaner and it will not introduce additional risk here (as the system implementation of these function will remain using time_t as input and double as output).

3. I will check with -Dlint, but note that passing flags withing visual studio is somewhat different, as Visual Studio does not work with makefile (it is able to do so, but all our own code is in solution/project files).
A quick check learned that our Linux systems (SLES11SP3, SLES12, RHAT6.6) has a too old gcc compiler. The -Wdate-time option requires gcc 4.9 as minimum. The compiler on SLES11 (gcc 4.3.4) is not smart enough to recognize a couple of other options. Since our customers rely on the default C/C++ runtime provided with the OS, I can't update the compiler (otherwise customers have to do the same for their runtime). Both Novell and RedHat do (usually) not replace a compiler once their Linux version becomes GA. Only newer major versions get equipped with a new(er) compiler, but even that compiler is (far) beyond the most recent gcc version. It's life.

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

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. That's why we turn this warning into an error.

Checking the code, I think the causes for the warnings are:
a. the code in tzparse() contains code paths that do no set dstname and thus the compiler issues a warning when dsttime is used in the strncpy() just before the tzparse () function returns.
b. the switch in transtime() does not have a default case, and thus the (pedantic) compiler issues a warning when value is used in the return statement.

I did not check all cases where the INITIALIZE macro is used, but changing it to the the #if part (although there is no lint on Windows) solves the previous warning.

3. date.c: Contains two calls to lose(2), where I think that close(2) is intended (lines 917 and 932), but in Linux all built well. The code is probably dead code?

4. For Windows/Visual Studio, there is no stdint.h file. In private.h, a typedef is used to define the int_fast32_t and int_fast64_t types, but not for the unsigned counterpart uint_fast64_t. Can you please add such type in private.h? The uint_fast64_t type is used in localtime.c, in detzcode64(). Note that Windows/Visual Studio also behaves somewhat odd (compared to (almost) all other non-Windows platforms: a long (in Visual Studio) remains 32-bit even when building in 64-bit mode.


Finally: Feel free to let me know if I forgot to answer a question of a previous email.

I'm busy applying the latest changes to our variant of the tzcode rules. Just building on one platform does not take much time, and sending logs is also done in a minute, but applying the changes takes more time. And building on different machines also takes more time. Due to our own changes, the differences in lines numbers are possibly confusing. The biggest differences are in localtime.c. I've to check (years of) our own version revision history for details. A colleague of me already told about a bugfix in the past. I'll let you know the details as soon as I know them.

BTW: my final goal is to check the (in our case) poor performance of opening/reading the zone files. I'm thinking off about building some LRU cache, as users of our product may often switch between a few different time zones. If you are interested in optimizations, please let me know.

Regards,
Kees

-----Original Message-----
From: Paul Eggert [mailto:eggert at cs.ucla.edu] 
Sent: Friday, July 24, 2015 18:35
To: Paul_Koning at Dell.com
Cc: Kees Dekker; Time zone mailing list
Subject: Re: [tz] suggestions for potential code improvements?

Paul_Koning at Dell.com wrote:
> If I read Kees’s comments correctly, he's talking about mismatches between declaration and definition.  That’s a different issue; the two should match.

Kees is not talking about this:

   int a (char *);
   int a (char const *v) {return *v;}

He's talking about this:

   int b (char *);
   int b (char *const v) {return *v;}

Although the first combination is invalid, the second one conforms to the C 
standard and this has been true since C89.  Any compiler that warns about the 
second combination is merely complaining about style; it's not a correctness issue.


More information about the tz mailing list