This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]