suggestions for potential code improvements?
Hi, I extracted the v2015e code and like to ask some things: 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. If you assign zero upon declaration, the INITIALIZE macro can go away. 2. In asctime_r() (asctime.c:72 and later) two arrays are used for week days. These arrays use a size of '3' for a 3-letter weekday. I expect to use an array of '4', because the '\0' character need to fit too, because the wn and mn variables are used in a call to sprintf() (line 106), where sprintf() normally requires to use 0-terminated strings. By using a '4'-sized array, this is automatically achieved. I don't know whether all sprintf() implementations for all operating systems respect the width/size specifier and allow non-0 terminated string. Now a size/width of '3' is specified, which is not needed if the strings are well terminated, or by accident used somewhere else. I really appreciate if you will do something with these suggestions. Regards, Kees
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.
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@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.
Kees Dekker wrote:
Will this patch become part of a successor version of the code?
I'm leaning against the idea, since it hurts performance very slightly and does not fix any bugs (it's just a style thing).
some compiler may complain about assigning a 3-character string literal (which is actually 4 bytes) to an array of 3-byte strings
The C standard explicitly says this is OK. Obviously a compiler can warn about it anyway, but normally we just use the latest stable GCC and pacify it with the settings in $(GCC_DEBUG_FLAGS). We don't have the time or inclination to pacify every pedantic compiler. On a case-by-case basis we can fix some issues (as I just did with 'const') but in the case of this 3-char literal pacifying the compiler would bloat the executable, and it's not worth it.
I will try to find a platform that complies about non-initialized variables.
Please don't forget to compile with -Dlint when you do that.
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@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.
Kees Dekker wrote:
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). ...
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.
That shouldn't matter. If the argument to a function is declared 'const', the 'const' attribute matters only in the body of the function; it is irrelevant to callers. This has been true ever since C was standardized in C89. No decent compiler should generate different code because of the extra 'const's here. That being said, I'd rather omit 'const' when used with locals as it's not useful enough to justify the code bloat. Since it's just a style thing and VS2013 is complaining we might as well omit it, so I installed the attached proposed patch in the experimental version on github <https://github.com/eggert/tz>; please give it a try.
2. The code in difftime.c:29, 37 and 50
Perhaps you're using some old version of difftime.c? Ours has only 48 lines. I suggest trying the latest version, preferably the experimental version on github.
(a subtraction of two doubles) results in a: conversion from 'const time_t' to 'double', possible loss of data warning.
'difftime' returns 'double', so (on some platforms, anyway) data will be lost. We can't change the return type of 'difftime' -- it's part of the C standard -- so people will just have to live with the data loss.
May be Visual Studio is somewhat pedantic in this case, but a cast will solve the issue.
Is the idea to change "return time1 - time0;" with "return (double) (time1 - time0);"? If so, I'd rather not. Casts are powerful in C and can lead to programming errors, so if there are two valid ways to write code, one with casts and one without, we should use the latter even if VS2013 complains about it.
If you like, I will also check the other files, but that will take additional time.
It shouldn't take long, no? Just compile the files and email us the compiler's messages. Should take less than a minute.
On Jul 24, 2015, at 12:10 PM, Paul Eggert <eggert@cs.ucla.edu> wrote:
Kees Dekker wrote:
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). ...
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.
That shouldn't matter. If the argument to a function is declared 'const', the 'const' attribute matters only in the body of the function; it is irrelevant to callers. This has been true ever since C was standardized in C89. No decent compiler should generate different code because of the extra 'const's here.
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. You’re correct that a caller can pass a non-const input into a const argument, that is of course legal, but that doesn’t appear to be what the complaint is about. paul
Paul_Koning@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.
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@cs.ucla.edu] Sent: Friday, July 24, 2015 18:35 To: Paul_Koning@Dell.com Cc: Kees Dekker; Time zone mailing list Subject: Re: [tz] suggestions for potential code improvements? Paul_Koning@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.
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.
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-o.... - 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@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.
Kees Dekker wrote:
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.
Yes, let's not worry about that. This sort of conversion is portable and standard-conforming C code, and inserting a cast would make the code harder to read and more error-prone.
Notes for localtime.c: - The functions typesequiv(), tzparse (), timesub(), increment_overflow(), normalize_overflow32() differ in const qualifiers between declarations and definitions.
Thanks, fixed in the attached patch #1.
- 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).
That size_t value should never exceed INT_MAX, so there shouldn't be a problem here. However, now that you mention it I see an unlikely possibility that stdlen would exceed INT_MAX. Fixed in attached patch #2.
Similar conversion errors are at lines 1197 and 1199 (because charcnt is an int, and stdlen and dstlen are of size_t type)
Patch #2 should also fix the actual bug here. I'm not so much worried about the VS diagnostics, if they're false alarms -- basically, VS appears to be complaining about all implicit conversions and this generates false alarms in tzcode. While we're in the neighborhood I found a minor bug in error-checking the newfangled angle-bracket abbreviations; fixed in attached patch#3.
and 1594 (idelta is an int), 1609 (second is an int, tdays a 64-bit time_t), 1616 (idays is an int, tdays not).
Line 1594 ("idelta = tdelta;") is safe, as the previous three lines guarantee that INT_MIN <= tdelta && tdelta <= INT_MAX. Similarly, line 1616 ("idays = tdays;") is safe, as tdays must be in the range 0..366 here (this is guaranteed by the previous while loop). Although line 1609 is also safe I see that it can be removed; it dates back to when the code was intended to be portable to platforms where time_t is a floating-point type; this never really worked and was removed in 2013 and this code is now unnecessary. Fixed in attached patch #4. I've installed the attached proposed patches into the experimental version on github.
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@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.
Kees Dekker wrote:
Regarding date.c:
Thanks for your comments. date.c is mostly a proof-of-concept for tzdata; as far as I know, no downstream package is actually using it, and it's intended more to demonstrate the API than anything else. That being said, we might as well keep date.c up-to-date (so to speak), so....
date.c is very *NIX centric. Windows does not have utmp.h (is not POSIX either)
The utmp.h-related code is very old, and predates POSIX. As POSIX standardized on utmpx.h in the 1990s, it's time to remove the old utmp.h code. I've done that in proposed patch #1 (attached). This should remove any need for ifdeffing utmp.h out.
I was a little bit surprised that no _WIN32 defines exist in your code...
There are many operating systems and they are constantly mutating, and we don't have the time or inclination to maintain code that depends on _WIN32, __WIN32__, __CYGWIN__, _AIX, __hpux, etc., etc. Instead, we have specific macros dealing with specific features (e.g., HAVE_UTMPX_H), and it's the responsibility of whoever's porting code to a particular platform to set the macros accordingly. The defaults are intended to be suitable for a standard POSIX environment. Now that you mention it, though, date.c has portability problems other than utmp, problems that also come from the fact that date.c was written before POSIX was standardized. While we're at it we might as well modernize date.c to use current POSIX interfaces in these other areas, so I did that in proposed patch #2 (attached). This removes some rarely-used options (usable only by 'root') in date.c, options that weren't portable anyway. These proposed patches are installed in the experimental version on Github; please base further comments on the Github version.
On Tuesday, July 28 2015, "Paul Eggert" wrote to "Kees Dekker, Time zone mailing list" saying:
+ date now sets the time of day via the POSIX clock_settime function + instead of via the older nonstandard settimeofday function.
clock_settime isn't actually that portable; notably, Mac OS X doesn't implement it. (It was part of the optional "Timers" extension to Posix in the Posix 2008 release, only making it into the core specification in the 2013 release.) -- Jonathan Lennox lennox@cs.columbia.edu
Jonathan Lennox wrote:
clock_settime isn't actually that portable; notably, Mac OS X doesn't implement it.
Ouch, thanks, I didn't know that. Let's simplify things further and remove all the code that tries to set the date. Setting the date is not germane to the tz project; the code is there only because it was in 4.2BSD, and that is not a particularly strong reason nowadays. Proposed patch attached.
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-o.... - 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@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.
Kees Dekker wrote:
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().
Thanks, should be fixed in the attached proposed patch (installed on Github).
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
As mentioned earlier there is no bug there. the compiler is merely complaining about style, and pacifying the compiler will require making the code a little larger at run-time; so let's leave the code alone.
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.
For warnings about implicit conversions I wouldn't worry unless there is a real bug. I don't see why MS-Windows' lack of support for gmtime_r is a problem: tzcode supplies its own gmtime_r implementation and this implementation should work.
Hi Paul, Thanks again. Sorry for the delay, I was one day off. Except the patch supplied to Jonathan Lennox, I was able to apply them all. Will I get the latest sources (including all previously sent patches) when I run: git clone https://github.com/eggert/tz.git? Anyhow, I created a simple example where I found that gcc does not recognize any mismatch in int types (between declaration and definition) with only const qualifiers are added. Also note the strange error regarding the [const] char * mismatch. See example below. Just try to compile with gcc -Wall -Wextra -o <output> <input-c-file-as-copied from this email>. If you find this gcc behavior odd too, I will check whether I can file this to the gcc/gnu.org folks. #include <stdio.h> static void f(const int i); static void g(int i); static void h(const char *s); int main(void) { f(3); g(4); h("A joke"); return 0; } static void f(int i) { printf("f-Input is: %d\n", i); } static void g(const int i) { printf("g-Input is: %d\n", i); } static void h(char *s) { printf("h-Input is: %s\n", s); } Regards, Kees -----Original Message----- From: Paul Eggert [mailto:eggert@cs.ucla.edu] Sent: Wednesday, July 29, 2015 03:46 To: Kees Dekker Cc: Time zone mailing list Subject: Re: [tz] suggestions for potential code improvements? Kees Dekker wrote:
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().
Thanks, should be fixed in the attached proposed patch (installed on Github).
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
As mentioned earlier there is no bug there. the compiler is merely complaining about style, and pacifying the compiler will require making the code a little larger at run-time; so let's leave the code alone.
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.
For warnings about implicit conversions I wouldn't worry unless there is a real bug. I don't see why MS-Windows' lack of support for gmtime_r is a problem: tzcode supplies its own gmtime_r implementation and this implementation should work.
Kees Dekker wrote:
Will I get the latest sources (including all previously sent patches) when I run: git clone https://github.com/eggert/tz.git?
Yes, the intent is that the experimental Github version reflects all the acceptable patches.
Anyhow, I created a simple example where I found that gcc does not recognize any mismatch in int types (between declaration and definition) with only const qualifiers are added. Also note the strange error regarding the [const] char * mismatch.
GCC is working correctly in your example, and there's nothing strange about it: it's what the C standard says. 'const' at the top level of an argument declaration is irrelevant as far as the caller is concerned. I discussed this here: http://mm.icann.org/pipermail/tz/2015-July/022519.html
Hi Paul, Remarks on date.c: The definitions of static functions oops() and iffy() do not have a declaration (raising e.g. 'oops' undefined; assuming extern returning int in Visual Studio (and probably other compilers). Can you please add two prototypes for these static functions? BTW. I think the two files that were not correctly patched are the Makefile and date.c. So my comments on date.c may be related to an outdated version. Remarks for zdump.c: You said that tzcode supplies its own gmtime_r(). Do you mean the onze in zdump.c itself? That one is only supplied if HAVE_LOCALTIME_R was not set/defined. Remarks for zic.c: There are still some inconsistent prototypes: inleap(),inlink(),rulesub(),outzone(),addtt(),yearistype(),byword(), oadd(),tadd(),rpytime(),newabbr(). If you can make them consistent (declaration & definition) it would be great. Regards, Kees -----Original Message----- From: Kees Dekker Sent: Thursday, July 30, 2015 15:19 To: 'Paul Eggert' Cc: Time zone mailing list Subject: RE: [tz] suggestions for potential code improvements? Hi Paul, Thanks again. Sorry for the delay, I was one day off. Except the patch supplied to Jonathan Lennox, I was able to apply them all. Will I get the latest sources (including all previously sent patches) when I run: git clone https://github.com/eggert/tz.git? Anyhow, I created a simple example where I found that gcc does not recognize any mismatch in int types (between declaration and definition) with only const qualifiers are added. Also note the strange error regarding the [const] char * mismatch. See example below. Just try to compile with gcc -Wall -Wextra -o <output> <input-c-file-as-copied from this email>. If you find this gcc behavior odd too, I will check whether I can file this to the gcc/gnu.org folks. #include <stdio.h> static void f(const int i); static void g(int i); static void h(const char *s); int main(void) { f(3); g(4); h("A joke"); return 0; } static void f(int i) { printf("f-Input is: %d\n", i); } static void g(const int i) { printf("g-Input is: %d\n", i); } static void h(char *s) { printf("h-Input is: %s\n", s); } Regards, Kees -----Original Message----- From: Paul Eggert [mailto:eggert@cs.ucla.edu] Sent: Wednesday, July 29, 2015 03:46 To: Kees Dekker Cc: Time zone mailing list Subject: Re: [tz] suggestions for potential code improvements? Kees Dekker wrote:
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().
Thanks, should be fixed in the attached proposed patch (installed on Github).
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
As mentioned earlier there is no bug there. the compiler is merely complaining about style, and pacifying the compiler will require making the code a little larger at run-time; so let's leave the code alone.
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.
For warnings about implicit conversions I wouldn't worry unless there is a real bug. I don't see why MS-Windows' lack of support for gmtime_r is a problem: tzcode supplies its own gmtime_r implementation and this implementation should work.
Kees Dekker wrote:
I think the two files that were not correctly patched are the Makefile and date.c. So my comments on date.c may be related to an outdated version.
Yes, that's correct. I suggest getting the latest version from github.
Remarks for zdump.c: You said that tzcode supplies its own gmtime_r(). Do you mean the onze in zdump.c itself? That one is only supplied if HAVE_LOCALTIME_R was not set/defined.
tzcode supplies two gmtime_r implementations. One in localtime.c, the other in zdump.c. zdump is supposed to be independent of the rest of the code, so it needs its own gmtime_r replacement if the system lacks gmtime_r and localtime.c is not being used. It's OK that zdump's gmtime_r is compiled when !HAVE_LOCALTIME_R, because every system that defines localtime_r also defines gmtime_r and vice versa.
Remarks for zic.c: There are still some inconsistent prototypes: inleap(),inlink(),rulesub(),outzone(),addtt(),yearistype(),byword(), oadd(),tadd(),rpytime(),newabbr(). If you can make them consistent (declaration & definition) it would be great.
Thanks, fixed in the attached patch (installed on github).
Addition: Localtime.c will have a gmtime_r() but of the system does not recognize gmtime_r(), there is also no prototype. So both localtime.c and zdump.c will suffer from it. And I would expect only one implementation (in localtime.c), not another one in zdump.c...not sure whether I'm not making a mistake. Regards, Kees -----Original Message----- From: Kees Dekker Sent: Thursday, July 30, 2015 16:42 To: 'Paul Eggert' Cc: 'Time zone mailing list' Subject: RE: [tz] suggestions for potential code improvements? Hi Paul, Remarks on date.c: The definitions of static functions oops() and iffy() do not have a declaration (raising e.g. 'oops' undefined; assuming extern returning int in Visual Studio (and probably other compilers). Can you please add two prototypes for these static functions? BTW. I think the two files that were not correctly patched are the Makefile and date.c. So my comments on date.c may be related to an outdated version. Remarks for zdump.c: You said that tzcode supplies its own gmtime_r(). Do you mean the onze in zdump.c itself? That one is only supplied if HAVE_LOCALTIME_R was not set/defined. Remarks for zic.c: There are still some inconsistent prototypes: inleap(),inlink(),rulesub(),outzone(),addtt(),yearistype(),byword(), oadd(),tadd(),rpytime(),newabbr(). If you can make them consistent (declaration & definition) it would be great. Regards, Kees -----Original Message----- From: Kees Dekker Sent: Thursday, July 30, 2015 15:19 To: 'Paul Eggert' Cc: Time zone mailing list Subject: RE: [tz] suggestions for potential code improvements? Hi Paul, Thanks again. Sorry for the delay, I was one day off. Except the patch supplied to Jonathan Lennox, I was able to apply them all. Will I get the latest sources (including all previously sent patches) when I run: git clone https://github.com/eggert/tz.git? Anyhow, I created a simple example where I found that gcc does not recognize any mismatch in int types (between declaration and definition) with only const qualifiers are added. Also note the strange error regarding the [const] char * mismatch. See example below. Just try to compile with gcc -Wall -Wextra -o <output> <input-c-file-as-copied from this email>. If you find this gcc behavior odd too, I will check whether I can file this to the gcc/gnu.org folks. #include <stdio.h> static void f(const int i); static void g(int i); static void h(const char *s); int main(void) { f(3); g(4); h("A joke"); return 0; } static void f(int i) { printf("f-Input is: %d\n", i); } static void g(const int i) { printf("g-Input is: %d\n", i); } static void h(char *s) { printf("h-Input is: %s\n", s); } Regards, Kees -----Original Message----- From: Paul Eggert [mailto:eggert@cs.ucla.edu] Sent: Wednesday, July 29, 2015 03:46 To: Kees Dekker Cc: Time zone mailing list Subject: Re: [tz] suggestions for potential code improvements? Kees Dekker wrote:
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().
Thanks, should be fixed in the attached proposed patch (installed on Github).
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
As mentioned earlier there is no bug there. the compiler is merely complaining about style, and pacifying the compiler will require making the code a little larger at run-time; so let's leave the code alone.
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.
For warnings about implicit conversions I wouldn't worry unless there is a real bug. I don't see why MS-Windows' lack of support for gmtime_r is a problem: tzcode supplies its own gmtime_r implementation and this implementation should work.
Kees Dekker wrote:
Localtime.c will have a gmtime_r() but of the system does not recognize gmtime_r(), there is also no prototype.
We can fix that in localtime.c by defining gmtime_r before using it, as in the attached proposed patch.
So both localtime.c and zdump.c will suffer from it.
I don't see why zdump.c suffers from any problem here. When zdump.c defines its own gmtime_r, it does so before using it.
And I would expect only one implementation (in localtime.c), not another one in zdump.c.
As I mentioned, zdump.c is intended to be useful even if localtime.c is not linked. That is why it needs its own implementation. See the comment about USE_LTZ.
Hi Paul, Thanks again. When I'm back from holiday I will continue working on localtime.c (we have made some changes and I was checking how to add a LRU list). If you like, I can share my findings (of course: once I've found something interesting that will improve the code). Regards, Kees -----Original Message----- From: Paul Eggert [mailto:eggert@cs.ucla.edu] Sent: Thursday, July 30, 2015 22:49 To: Kees Dekker Cc: Time zone mailing list Subject: Re: [tz] suggestions for potential code improvements? Kees Dekker wrote:
Localtime.c will have a gmtime_r() but of the system does not recognize gmtime_r(), there is also no prototype.
We can fix that in localtime.c by defining gmtime_r before using it, as in the attached proposed patch.
So both localtime.c and zdump.c will suffer from it.
I don't see why zdump.c suffers from any problem here. When zdump.c defines its own gmtime_r, it does so before using it.
And I would expect only one implementation (in localtime.c), not another one in zdump.c.
As I mentioned, zdump.c is intended to be useful even if localtime.c is not linked. That is why it needs its own implementation. See the comment about USE_LTZ.
participants (4)
-
Jonathan Lennox -
Kees Dekker -
Paul Eggert -
Paul_Koning＠dell.com