diff mbox

time: in strptime(), make %z accept [+-]HH:MM time zones

Message ID 1422372004-32522-1-git-send-email-Vincent.Bernat@exoscale.ch
State Superseded
Delegated to: Mike Frysinger
Headers show

Commit Message

Vincent Bernat Jan. 27, 2015, 3:20 p.m. UTC
From: Vincent Bernat <vincent@bernat.im>

In ISO 8601, +03:30 is a valid time zone. Currently, strptime() only
parses it as a 2-digit time zone an believes this is +03:00. This change
makes it accept a single semi-colon.

This fix BZ #17887.
---
 ChangeLog            |  3 +++
 time/strptime_l.c    | 18 ++++++++++++++----
 time/tst-strptime2.c |  9 +++++++++
 3 files changed, 26 insertions(+), 4 deletions(-)

Comments

Mike Frysinger March 6, 2015, 10:21 a.m. UTC | #1
On 27 Jan 2015 16:20, Vincent Bernat wrote:
> In ISO 8601, +03:30 is a valid time zone. Currently, strptime() only
> parses it as a 2-digit time zone an believes this is +03:00. This change
> makes it accept a single semi-colon.

err, nowhere that i see here do you parse a semi-colon.  i guess you meant
"colon" ?

i think you're colliding with the fix for BZ #16141.  i'd prefer to merge
that first though, so you might want to rebase once that's done.

> This fix BZ #17887.

you should include [BZ #17887] in the subject line too

> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -3,6 +3,9 @@
>  	[BZ #17886]
>  	* time/strptime_l.c: Make %z accept Z as a valid time zone.
>  
> +	[BZ #17887]
> +	* time/strptime_l.c: Make %z accept [+-]HH:MM time zones.

each patch should get a standalone entry.  that means you'd add another 
date/name line above this.

also you should scope your changes a bit.  that means this would be:
	* time/strptime_l.c (__strptime_internal): .....

> +	  /* We recognize four formats: 1. if two digits are given,
> +	     these specify hours.  2. If fours digits are used,
> +	     minutes are also specified. 3. A semi-colon can be used
> +	     to separate the two groups of two digits (HH:MM). 4. 'Z'
> +	     is equivalent to +0000. */

needs two spaces after the periods

> @@ -765,8 +767,16 @@ __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM)
>  	      return NULL;
>  	    bool neg = *rp++ == '-';
>  	    int n = 0;
> -	    while (n < 4 && *rp >= '0' && *rp <= '9')
> +	    while (n < 4 &&
> +                   ((*rp >= '0' && *rp <= '9') ||
> +                    (*rp == ':' && n == 2)))
>  	      {
> +                if (*rp == ':')
> +                  {
> +                    rp++;
> +                    if (!(*rp >= '0' && *rp <= '9'))
> +                      return NULL;
> +                  }
>  		val = val * 10 + *rp++ - '0';
>  		++n;
>  	      }

indentation is broken -- needs to start with a tab

not exactly a new issue, but probably should be using isdigit() here

i think the loop might be cleaner if you didn't duplicate the checks.
i.e. something like this (ignoring style, and this is untested):
	while (n < 4) {
		if (n == 2 && *rp == ':')
			++rp;
		if (!isdigit (*rp))
			return NULL;
		val = val * 10 + *rp++ - '0';
		++n;
	}
-mike
Vincent Bernat March 6, 2015, 11:06 a.m. UTC | #2
❦  6 mars 2015 05:21 -0500, Mike Frysinger <vapier@gentoo.org> :

> i think you're colliding with the fix for BZ #16141.  i'd prefer to merge
> that first though, so you might want to rebase once that's done.

OK, I am waiting for BZ #16141 to be pushed before pushing the patches
for BZ #17886 and BZ #17887 (since they are co-dependant).
Vincent Bernat April 20, 2015, 11:42 a.m. UTC | #3
❦  6 mars 2015 04:21 -0500, Mike Frysinger <vapier@gentoo.org> :

> i think you're colliding with the fix for BZ #16141.  i'd prefer to merge
> that first though, so you might want to rebase once that's done.

It seems there is no more activity on #16141. What should I do?
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index 24c1a74c963d..0f3e4bdc72a8 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -3,6 +3,9 @@ 
 	[BZ #17886]
 	* time/strptime_l.c: Make %z accept Z as a valid time zone.
 
+	[BZ #17887]
+	* time/strptime_l.c: Make %z accept [+-]HH:MM time zones.
+
 2015-01-27  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>
 
 	* iconv/loop.c: Suppress array out of bound warning caused by GCC
diff --git a/time/strptime_l.c b/time/strptime_l.c
index 78882ec39aed..3326e110d928 100644
--- a/time/strptime_l.c
+++ b/time/strptime_l.c
@@ -749,9 +749,11 @@  __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM)
 	    rp++;
 	  break;
 	case 'z':
-	  /* We recognize three formats: if two digits are given, these
-	     specify hours.  If fours digits are used, minutes are
-	     also specified. 'Z' is equivalent to +0000. */
+	  /* We recognize four formats: 1. if two digits are given,
+	     these specify hours.  2. If fours digits are used,
+	     minutes are also specified. 3. A semi-colon can be used
+	     to separate the two groups of two digits (HH:MM). 4. 'Z'
+	     is equivalent to +0000. */
 	  {
 	    val = 0;
 	    while (ISSPACE (*rp))
@@ -765,8 +767,16 @@  __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM)
 	      return NULL;
 	    bool neg = *rp++ == '-';
 	    int n = 0;
-	    while (n < 4 && *rp >= '0' && *rp <= '9')
+	    while (n < 4 &&
+                   ((*rp >= '0' && *rp <= '9') ||
+                    (*rp == ':' && n == 2)))
 	      {
+                if (*rp == ':')
+                  {
+                    rp++;
+                    if (!(*rp >= '0' && *rp <= '9'))
+                      return NULL;
+                  }
 		val = val * 10 + *rp++ - '0';
 		++n;
 	      }
diff --git a/time/tst-strptime2.c b/time/tst-strptime2.c
index c22967a93026..1c0958fc8bc0 100644
--- a/time/tst-strptime2.c
+++ b/time/tst-strptime2.c
@@ -13,16 +13,25 @@  static const struct
     { "1113472456 -1000", -36000 },
     { "1113472456 +10", 36000 },
     { "1113472456 -10", -36000 },
+    { "1113472456 +10:00", 36000 },
+    { "1113472456 -10:00", -36000 },
     { "1113472456 +1030", 37800 },
     { "1113472456 -1030", -37800 },
+    { "1113472456 +10:30", 37800 },
+    { "1113472456 -10:30", -37800 },
     { "1113472456 +0030", 1800 },
     { "1113472456 -0030", -1800 },
     { "1113472456  Z", 0 },
     { "1113472456 -1330", LONG_MAX },
     { "1113472456 +1330", LONG_MAX },
+    { "1113472456 -13:30", LONG_MAX },
+    { "1113472456 +13:30", LONG_MAX },
     { "1113472456 -1060", LONG_MAX },
     { "1113472456 +1060", LONG_MAX },
+    { "1113472456 -10:60", LONG_MAX },
+    { "1113472456 +10:60", LONG_MAX },
     { "1113472456  1030", LONG_MAX },
+    { "1113472456  10:30", LONG_MAX },
   };
 #define ntests (sizeof (tests) / sizeof (tests[0]))