[illumos-Developer] webrev: fix for 317 strptime needs %Z support

[Mike Riley]

On 12/03/10 20:32, Garrett D'Amore wrote:
> http://mexico.purplecow.org/gdamore/webrev/strptime/
>
> I would appreciate a timely review of this set of fixes for libc.
>
> This includes adding proper support for parsing "foreign" timezone value
> (with %z), and also adds support for %s in strftime and strptime, which
> are supported on BSD and Linux.
>
> As part of this, this also introduces timegm, which is also present on
> the other non-Solaris FOSS'.  (It does so "safely", I believe, without
> breaking compliance with any standards.)
>
> Finally, while here, I have also cleaned up the code in strptime, using
> NULL instead of 0, and removing pointless casts and pointless checks
> against a terminating byte prior to calling ctype macros.  (These macros
> properly handle the 0 byte -- treating it as a control character.)  The
> net effect there ins improved readability of that code.
>
> I have a test program that I used to test this code, that I'll be happy
> to share on request.
>
> Note that the addition of this functionality means that applications
> that are compiled to use these functions or features may not work
> properly on Solaris.  (This will not affect *illumos*'s ability to run
> applications compiled on Solaris, of course.)
>
> 	- Garrett

Hi Garret,

If you are introducing a new function, like timegm(), could you also please include an updated 
man page entry?  I found something suitable here: http://en.wikipedia.org/wiki/Time.h

I would also like to see the definition for what %z and %s are supposed to be doing here in an 
updated man page.  Otherwise I have to assume your code is doing the right thing, or else 
compare it to some equivalent code from another OS.

In time.h I would move the definition from line 200 to line 184, to reduce the number of #if's 
that we have to deal with.  The one seems to have the exact same conditions it is checking for. 
  The second definition does not have such an equivalent check, so it should stay as you have 
it.  I hate to have any more conditional compilation checks than we absolutely have to.

In strptime.c:

Line 429: You left a check for "*buf == NULL" (formerly "*buf == 0"), but that check is not 
needed inside your switch statement any more.  You had removed it for all the previous ones, 
which certainly reduces the code size nicely.

Good cleanup of the syntax in there as well by removing the unneeded casts and changing the "0" 
to "NULL" for the pointer checks and returns.

Other than those nits I think this looks fine to me.

On a separate note:

Can we provide an OpenGrok page on the web site?  I find that it is much easier to do code 
reviews if that is available.  It helps in researching uses of routines you are not familiar 
with, plus looking for other areas in the system that might be affected by changes being made 
that use the changed code.

Mike