Re: Cygwin strptime() is missing "%s" which strftime() has

[Brian Inglis <Brian dot Inglis at Shaw dot ca>]

On 2017-07-25 03:16, Corinna Vinschen wrote:
> Hi Brian,
> 
> On Jul 24 14:41, Brian Inglis wrote:
>> On Mon, 24 Jul 2017 02:32:14 -0700, Corinna Vinschen wrote:> On Jul 23 22:07,
>>> In this case I have a nit, but this should be discussed on the right
>>> mailing list so all affected parties can chime in.  Hint: strtoimax is
>>> not available on all platforms yet (patches still in limbo)...
>>
>> Figured there would need to be some tweaks for newlib platforms, compilers, and
>> style, so made some changes, attached another diff for discussion, before
>> submitting a patch.
>> Let me know if you want conditionals or declarations changed, hoisted to
>> function start, case braces removed, other issues?
> 
> See below.
> 
>> diff --git a/newlib/libc/time/strptime.c b/newlib/libc/time/strptime.c
>> index c0861eb87..112227e40 100644
>> --- a/newlib/libc/time/strptime.c
>> +++ b/newlib/libc/time/strptime.c
>> @@ -38,6 +38,8 @@
>>  #include <strings.h>
>>  #include <ctype.h>
>>  #include <stdlib.h>
>> +#include <inttypes.h>
>> +#include <limits.h>
>>  #include "../locale/setlocale.h"
>>  
>>  #define _ctloc(x) (_CurrentTimeLocale->x)
>> @@ -230,6 +232,13 @@ strptime_l (const char *buf, const char *format, struct tm *timeptr,
>>  		buf = s;
>>  		ymd |= SET_MDAY;
>>  		break;
>> +	    case 'F' :		/* %Y-%m-%d */
>> +		s = strptime_l (buf, "%Y-%m-%d", timeptr, locale);
>> +		if (s == NULL)
>> +		    return NULL;
>> +		buf = s;
>> +		ymd |= SET_YMD;
>> +		break;
>>  	    case 'H' :
>>  	    case 'k' :
>>  		ret = strtol_l (buf, &s, 10, locale);
>> @@ -300,6 +309,31 @@ strptime_l (const char *buf, const char *format, struct tm *timeptr,
>>  		    return NULL;
>>  		buf = s;
>>  		break;
>> +	    case 's' : {
>> +#if defined(INTMAX_MAX)
>> +#  define BIG_T		intmax_t
>> +#  define STRTOBIG	strtoimax
>> +#elif defined(LLONG_MAX)
>> +#  define BIG_T		long long
>> +#  define STRTOBIG	strtoll
>> +#else
>> +#  define BIG_T		long
>> +#  define STRTOBIG	strtol
>> +#endif
> 
> I don't think we need to use intmax_t at all here.  Checking for
> LLONG_MAX should be sufficient.  However, this is strptime_l.  so you
> should use strtoll_l/strtol_l, just like the rest of the function.
> 
> On second thought, do we have to do this at all?  Our time_t is always
> long anyway so using just strtol_l and checking for ERANGE should be
> sufficient:
> 
>   int old_errno = _REENT->_errno;
>   sec = strtol_l (buf, &s, 10);
>   int new_errno = _REENT->_errno;
>   _REENT->_errno = old_errno;
>   if (s == buf || new_errno == ERANGE || etc...
> 
>> +		    BIG_T sec;
>> +		    time_t t;
>> +
>> +		    sec = STRTOBIG (buf, &s, 10);
>> +		    t = (time_t)sec;
>> +		    if (s == buf
>> +			|| (BIG_T)t != sec
>> +			|| localtime_r (&t, timeptr) != timeptr)

Is time_t always long on all newlib platforms, or could it be long long in some
environments/memory models e.g. Windows 64 VS/MinGW LLP64/IL32P64 vs Cygwin/Unix
LP64/I32LP64?
Could/should we keep the strtol[l] options and use the ..._l variants?

Can't we just use errno, as shouldn't that be mapped to _REENT->_errno in this
context if required, or can it/does it need to be explicit?
These are locale-dependent ..._l functions not reentrant ..._r functions, and
there is no "#include <reent.h>"?

Don't we need to save and zero errno to distinguish a new error, and restore if
it stays zero, rather than just pick up the current value, and assume if it
is/was ERANGE it's bad?

> Shouldn't this be gmtime_r?
> 
>  %s     The number of seconds since the Epoch, 1970-01-01 00:00:00 +0000
> 	(UTC).  Leap seconds are not counted unless leap second  support
> 	is available.

The input is seconds since the epoch, but the interpretation in struct tm
depends on the locale, so we use localtime_r(3).
The timezone may be set in the environment or locale, and may be UTC.
If you want gmtime/UTC you set TZ=UTC0, TZ=Etc/UTC, which should override/change
locale LC_TIME, as would setting %z with value +0000 or %Z with values UTC or Z,
where that is supported by strptime_l(3) (i.e. not here).

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada