diff mbox

[1/2] time: in strptime(), make %z accept Z as a time zone [BZ #17886]

Message ID 87si6ujiyq.fsf@zoro.exoscale.ch
State Superseded
Headers show

Commit Message

Vincent Bernat Sept. 4, 2015, 3:35 p.m. UTC
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.
---
 ChangeLog            | 6 ++++++
 time/strptime_l.c    | 9 +++++++--
 time/tst-strptime2.c | 7 +++++++
 3 files changed, 20 insertions(+), 2 deletions(-)

Comments

James Perkins Sept. 11, 2015, 5:21 p.m. UTC | #1
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
Vincent Bernat Sept. 16, 2015, 7:57 a.m. UTC | #2
❦ 11 septembre 2015 10:21 -0700, James Perkins <james@loowit.net> :

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

It is unrelated to the patch and this is also how this is done in
get_number() macro. I prefer to leave this as is if you don't mind.

>> +	    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;
> +	      }

Agreed.

Updated patchset will come shortly.
diff mbox

Patch

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.
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;
+	    if (*rp == 'Z')
+	      {
+		++rp;
+		break;
+	      }
 	    if (*rp != '+' && *rp != '-')
 	      return NULL;
 	    bool neg = *rp++ == '-';
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),