Here are some proposed very minor code tuneups for the tz code. While accumulating these I noticed a portability bug in localtime.c; the expression `(t < 0) ? 0 : ((time_t) 1 << bits)' yields undefined results if time_t is unsigned, since `bits' is the number of bits in a time_t in that case. The patch below fixes this bug as part of the tuneups. =================================================================== RCS file: RCS/tzfile.h,v retrieving revision 1995.3 retrieving revision 1995.3.1.1 diff -c -r1995.3 -r1995.3.1.1 *** tzfile.h 1995/03/11 17:55:58 1995.3 --- tzfile.h 1996/01/15 06:22:59 1995.3.1.1 *************** *** 153,159 **** ** that will probably do. */ ! #define isleap(y) ((((y) % 4) == 0 && ((y) % 100) != 0) || ((y) % 400) == 0) #ifndef USG --- 153,159 ---- ** that will probably do. */ ! #define isleap(y) (((y) % 4) == 0 && (((y) % 100) != 0 || ((y) % 400) == 0)) #ifndef USG =================================================================== RCS file: RCS/difftime.c,v retrieving revision 1994.8 retrieving revision 1994.8.1.1 diff -c -r1994.8 -r1994.8.1.1 *** difftime.c 1994/12/09 04:33:32 1994.8 --- difftime.c 1996/01/15 06:22:59 1994.8.1.1 *************** *** 43,51 **** /* ** Repair delta overflow. */ ! hibit = 1; ! while ((hibit <<= 1) > 0) ! continue; /* ** The following expression rounds twice, which means ** the result may not be the closest to the true answer. --- 43,49 ---- /* ** Repair delta overflow. */ ! hibit = (~ (time_t) 0) << (TYPE_BIT(time_t) - 1); /* ** The following expression rounds twice, which means ** the result may not be the closest to the true answer. *************** *** 65,74 **** ** This problem occurs only with very large differences. ** It's too painful to fix this portably. ** We are not alone in this problem; ! ** many C compilers round twice when converting ** large unsigned types to small floating types, ** so if time_t is unsigned the "return delta" above ! ** has the same double-rounding problem. */ return delta - 2 * (long_double) hibit; } --- 63,72 ---- ** This problem occurs only with very large differences. ** It's too painful to fix this portably. ** We are not alone in this problem; ! ** some C compilers round twice when converting ** large unsigned types to small floating types, ** so if time_t is unsigned the "return delta" above ! ** has the same double-rounding problem with those compilers. */ return delta - 2 * (long_double) hibit; } =================================================================== RCS file: RCS/localtime.c,v retrieving revision 1995.7 retrieving revision 1995.7.1.1 diff -c -r1995.7 -r1995.7.1.1 *** localtime.c 1995/10/30 15:32:46 1995.7 --- localtime.c 1996/01/15 06:22:59 1995.7.1.1 *************** *** 1163,1175 **** tmp->tm_hour = (int) (rem / SECSPERHOUR); rem = rem % SECSPERHOUR; tmp->tm_min = (int) (rem / SECSPERMIN); ! tmp->tm_sec = (int) (rem % SECSPERMIN); ! if (hit) ! /* ! ** A positive leap second requires a special ! ** representation. This uses "... ??:59:60" et seq. ! */ ! tmp->tm_sec += hit; tmp->tm_wday = (int) ((EPOCH_WDAY + days) % DAYSPERWEEK); if (tmp->tm_wday < 0) tmp->tm_wday += DAYSPERWEEK; --- 1163,1173 ---- tmp->tm_hour = (int) (rem / SECSPERHOUR); rem = rem % SECSPERHOUR; tmp->tm_min = (int) (rem / SECSPERMIN); ! /* ! ** A positive leap second requires a special ! ** representation. This uses "... ??:59:60" et seq. ! */ ! tmp->tm_sec = (int) (rem % SECSPERMIN) + hit; tmp->tm_wday = (int) ((EPOCH_WDAY + days) % DAYSPERWEEK); if (tmp->tm_wday < 0) tmp->tm_wday += DAYSPERWEEK; *************** *** 1344,1358 **** /* ** Calculate the number of magnitude bits in a time_t ** (this works regardless of whether time_t is ! ** signed or unsigned, though lint complains if unsigned). */ ! for (bits = 0, t = 1; t > 0; ++bits, t <<= 1) ! continue; /* ** If time_t is signed, then 0 is the median value, ! ** if time_t is unsigned, then 1 << bits is median. */ ! t = (t < 0) ? 0 : ((time_t) 1 << bits); for ( ; ; ) { (*funcp)(&t, offset, &mytm); dir = tmcomp(&mytm, &yourtm); --- 1342,1355 ---- /* ** Calculate the number of magnitude bits in a time_t ** (this works regardless of whether time_t is ! ** signed or unsigned). */ ! bits = TYPE_BIT(time_t) - TYPE_SIGNED(time_t); /* ** If time_t is signed, then 0 is the median value, ! ** if time_t is unsigned, then 1 << (bits - 1) is median. */ ! t = TYPE_SIGNED(time_t) ? 0 : ((time_t) 1 << (TYPE_BIT(time_t) - 1)); for ( ; ; ) { (*funcp)(&t, offset, &mytm); dir = tmcomp(&mytm, &yourtm);
Date: Sun, 14 Jan 1996 22:37:20 -0800 From: Paul Eggert <eggert@twinsun.com> Message-ID: <199601150637.WAA15042@der.twinsun.com> While accumulating these I noticed a portability bug in localtime.c; the expression `(t < 0) ? 0 : ((time_t) 1 << bits)' yields undefined results if time_t is unsigned, since `bits' is the number of bits in a time_t in that case. The patch below fixes this bug as part of the tuneups. It is a "portability bug" in the sense that it is simply wrong if time_t is unsigned (but OK for signed time_t). I am not sure how this happened - I actually had unsigned time_t for a while (and it was in some BSD beta release ages ago, before people west of Greenwich complained about what that meant to ((time_t)0) when converted to local time... That is, once upon a time, this code was actually tested, and worked, with unsigned time_t, somehow it seems to have suffered some bit rot... Unfortunately I don't have any very ancient versions to look see what happened there. (bits - 1) is clearly the right thing (1 << bits) is going to be either 0 or 1 on most architectures (but might be anything). kre
participants (2)
-
Paul Eggert -
Robert Elz