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

[Garrett D'Amore]

On Sat, 2010-12-04 at 10:10 -0800, Mike Riley wrote:
> 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 could... but I have no where to store such a man page at present.
That is a different higher level question -- there are many points in
the APIs where we simply have no man pages at all.  I'll file a bug to
track this for future documentation efforts, so it isn't lost.

> 
> 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.

%z takes the numeric timezone offset, as [+-]HHMM  This is documented in
strftime(3c) already.

%s is the seconds since the UNIX epoch.  (I.e. the value of
ddi_get_time())

This logic was basically copied from the FreeBSD code.  Note that no
attribution is required, because we are already attributing the FreeBSD
code since that was the origin of this file to begin with.

> 
> 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.
> 

Agreed on the first.  On the second I'm going to move it to cftime().
This is for K&R C -- I really want to clean that whole section out, but
that is something for a later follow up.  But I don't have to worry
about standard conformance for K&R, so it can live with cftime() and
ascftime().


> 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.

That one at 429 is not removable.  Because the check is for *either*
*buf == NULL or isspace(*buf).  You need both checks.  (The other ones I
removed were "if (!isdigit(*buf)) in nature, which is accurate.

> 
> 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.

Thanks!

> 
> 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.

http://src.illumos.org/

	- Garrett
> 
> Mike
> 
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer