This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 1/2] time: in strptime(), make %z accept Z as a time zone [BZ #17886]
- From: James Perkins <james at loowit dot net>
- To: libc-alpha at sourceware dot org, Vincent Bernat <Vincent dot Bernat at exoscale dot ch>
- Cc: Mike Frysinger <vapier at gentoo dot org>
- Date: Fri, 11 Sep 2015 10:21:55 -0700
- Subject: Re: [PATCH 1/2] time: in strptime(), make %z accept Z as a time zone [BZ #17886]
- Authentication-results: sourceware.org; auth=none
- References: <cover dot 1441380709 dot git dot Vincent dot Bernat at exoscale dot ch> <87si6ujiyq dot fsf at zoro dot exoscale dot ch>
Some minor nits below...
On Fri, Sep 04, 2015 at 05:35:25PM +0200, Vincent Bernat wrote:
> In ISO 8601, the timezone can be 'Z' instead of using
> digits. 2014-08-17T12:33:12+0000 is often expressed as
> 2014-08-17T12:33:12Z.
>
> This fixes BZ #17886.
OK
> ---
> ChangeLog | 6 ++++++
> time/strptime_l.c | 9 +++++++--
> time/tst-strptime2.c | 7 +++++++
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/ChangeLog b/ChangeLog
> index ab0031d9dbe2..490cfbef5025 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,3 +1,9 @@
> +2015-09-04 Vincent Bernat <vincent@bernat.im>
> +
> + [BZ #17886]
> + * time/strptime_l.c (__strptime_internal): Make %z accept Z as a
> + valid time zone.
> +
> 2015-09-03 Roland McGrath <roland@hack.frob.com>
>
> * elf/Makefile (test-xfail-tst-protected1a): New variable.
The ChangeLog diff will most likely not apply to the current master
head commit. Perhaps this is why the ChangeLog is typically included
at the end of the git header comment instead of as part of the diff
(as I understand it the official committers will modify the ChangeLog
on your behalf).
> diff --git a/time/strptime_l.c b/time/strptime_l.c
> index 4203ad81a0f3..8fdd4e8e3f7a 100644
> --- a/time/strptime_l.c
> +++ b/time/strptime_l.c
> @@ -749,13 +749,18 @@ __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM)
> rp++;
> break;
> case 'z':
> - /* We recognize two formats: if two digits are given, these
> + /* We recognize three formats: if two digits are given, these
> specify hours. If fours digits are used, minutes are
> - also specified. */
> + also specified. 'Z' is equivalent to +0000. */
> {
> val = 0;
> while (ISSPACE (*rp))
> ++rp;
OK.
However, note that val is not used until the loop that accumulates a
sum in val. As an optimization/localization it could be moved down past
the tests for Z, + and -. e.g.
+ val = 0;
while (n < 4 && *rp >= '0' && *rp <= '9')
.... accumulate digits in val ...
> + if (*rp == 'Z')
> + {
> + ++rp;
> + break;
> + }
> if (*rp != '+' && *rp != '-')
> return NULL;
> bool neg = *rp++ == '-';
I've thought about this early break a bit more and I think it's
incomplete. strptime will not set tm fields if they are not valid
input. However, it *does* set the tm fields if it parses them
correctly. So for completeness, you probably want to do this:
+ if (*rp == 'Z')
+ {
+ ++rp;
+ tm->tm_gmtoff = 0;
+ break;
+ }
> diff --git a/time/tst-strptime2.c b/time/tst-strptime2.c
> index 5b06a63ba4c3..cfb7d70c4cc2 100644
> --- a/time/tst-strptime2.c
> +++ b/time/tst-strptime2.c
> @@ -155,6 +155,13 @@ do_test (void)
> expect = LONG_MAX;
> result |= compare (buf, expect, nresult);
>
> + /* Create and test input string with "Z" input (valid format).
> + Expect tm_gmtoff of 0. */
> +
> + sprintf (buf, "%s Z", dummy_string);
> + expect = 0;
> + result |= compare (buf, expect, nresult);
> +
> /* Create and test input strings with sign and digits:
> 0 digits (invalid format),
> 1 digit (invalid format),
Test looks good.
James