diff mbox

[2/2] time: in strptime(), make %z accept [+-]HH:MM tz [BZ #17887]

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

Commit Message

Vincent Bernat Sept. 4, 2015, 3:35 p.m. UTC
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 colon.

This fixes BZ #17887.
---
 ChangeLog            |  6 ++++++
 time/strptime_l.c    | 10 +++++++---
 time/tst-strptime2.c | 21 ++++++++++++++++-----
 3 files changed, 29 insertions(+), 8 deletions(-)

Comments

James Perkins Sept. 11, 2015, 5:43 p.m. UTC | #1
On Fri, Sep 04, 2015 at 05:35:53PM +0200, 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 colon.
> 
> This fixes BZ #17887.
> ---
>  ChangeLog            |  6 ++++++
>  time/strptime_l.c    | 10 +++++++---
>  time/tst-strptime2.c | 21 ++++++++++++++++-----
>  3 files changed, 29 insertions(+), 8 deletions(-)

OK

> 
> diff --git a/ChangeLog b/ChangeLog
> index 490cfbef5025..177d80cad358 100644
> --- a/ChangeLog
> +++ b/ChangeLog
> @@ -1,5 +1,11 @@
>  2015-09-04  Vincent Bernat  <vincent@bernat.im>
>  
> +	[BZ #17887]
> +	* time/strptime_l.c (__strptime_internal): Make %z accept
> +	[+-]HH:MM time zones.
> +
> +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.

Content is good.

Same comment as prior message - what is the maintainer preference for
ChangeLog in the diff vs. at the end of the commit header comment?

> diff --git a/time/strptime_l.c b/time/strptime_l.c
> index 8fdd4e8e3f7a..07aabc8deb54 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 colon can be used
> +	     to separate the two groups of two digits (HH:MM).  4. 'Z'
> +	     is equivalent to +0000.  */

Content is good. Formatting request: Could the 4. 'Z' at least be moved
to the next line?  For clarity, you could just split them out one to a
line, example:

+	  /* We recognize four formats:
+	     1. Two digits specify hours.
+	     2. Four digits specify hours and minutes.
+	     3. Two digits, ':', and two digits specify hours and minutes.
+	     4. 'Z' is equivalent to +0000.  */

>  	  {
>  	    val = 0;
>  	    while (ISSPACE (*rp))
> @@ -769,6 +771,8 @@ __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM)
>  	      {
>  		val = val * 10 + *rp++ - '0';
>  		++n;
> +		if (*rp == ':' && n == 2)
> +		  ++rp;
>  	      }
>  	    if (n == 2)
>  	      val *= 100;

Functionally OK, but needs to also check that the next character after
rp is a digit so that we will accept input like "00:30" and reject input
like "00::::30". Perhaps something like:

+		if (*rp == ':' && n == 2 && isdigit(*(rp + 1)))
+		  ++rp;

> diff --git a/time/tst-strptime2.c b/time/tst-strptime2.c
> index cfb7d70c4cc2..318fab973a36 100644
> --- a/time/tst-strptime2.c
> +++ b/time/tst-strptime2.c
> @@ -34,7 +34,8 @@ static bool verbose;
>     Note that a valid timezone string contains the following fields:
>       Sign field consisting of a '+' or '-' sign,
>       Hours field in two decimal digits, and
> -     optional Minutes field in two decimal digits.
> +     optional Minutes field in two decimal digits. Optionally,
> +     a ':' is used to seperate hours and minutes.
>  
>     This function may write test strings with minutes values outside
>     the valid range 00-59.  These are invalid strings and useful for
> @@ -55,7 +56,7 @@ static bool verbose;
>     range of 00 to 59.  */
>  
>  static long int
> -mkbuf (char *buf, bool neg, unsigned int hhmm, size_t ndigits)
> +mkbuf (char *buf, bool neg, bool colon, unsigned int hhmm, size_t ndigits)
>  {
>    const int mm_max = 59;
>    char sign = neg ? '-' : '+';
> @@ -65,7 +66,9 @@ mkbuf (char *buf, bool neg, unsigned int hhmm, size_t ndigits)
>    long int expect = LONG_MAX;
>  
>    i = sprintf (buf, "%s %c", dummy_string, sign);
> -  snprintf (buf + i, ndigits + 1, "%04u", hhmm);
> +  snprintf (buf + i,
> +            (colon && ndigits >= 2) ? ndigits + 2 : ndigits + 1,
> +            "%02u%s%02u", hh, colon ? ":" : "", mm);

Functionally OK, but formatting issue.  The style in this file is to use
tab for every 8 leading whitespace and the patch uses 8 spaces instead.
indent -npro can help here.

>  
>    if (mm <= mm_max && (ndigits == 2 || ndigits == 4))
>      {
> @@ -176,10 +179,18 @@ do_test (void)
>        {
>  	/* Test both positive and negative signs.  */
>  
> -	expect = mkbuf (buf, false, hhmm, ndigits);
> +	expect = mkbuf (buf, false, false, hhmm, ndigits);
>  	result |= compare (buf, expect, nresult);
>  
> -	expect = mkbuf (buf, true, hhmm, ndigits);
> +	expect = mkbuf (buf, true, false, hhmm, ndigits);
> +	result |= compare (buf, expect, nresult);
> +

OK

> +        /* Test with colon as well.  */

Formatting issue.  Leading whitespace should be a tab instead of 8 spaces.

> +
> +	expect = mkbuf (buf, false, true, hhmm, ndigits);
> +	result |= compare (buf, expect, nresult);
> +
> +	expect = mkbuf (buf, true, true, hhmm, ndigits);
>  	result |= compare (buf, expect, nresult);
>        }

The logic above is OK when ndigits >= 3.  When ndigits < 3, there is no
point to calling mkbuf with a colon parameter of true (it is the same
test as setting it false).  Better would be:

+
+ 	if (ndigits >= 3)
+ 	  {
+	    expect = mkbuf (buf, false, true, hhmm, ndigits);
+	    result |= compare (buf, expect, nresult);
+
+	    expect = mkbuf (buf, true, true, hhmm, ndigits);
+ 	    result |= compare (buf, expect, nresult);
+ 	  }
        }

Coming along well.

James
Vincent Bernat Sept. 16, 2015, 8:16 a.m. UTC | #2
❦ 11 septembre 2015 10:43 -0700, James Perkins <opalmirror@gmail.com> :

> Content is good. Formatting request: Could the 4. 'Z' at least be moved
> to the next line?  For clarity, you could just split them out one to a
> line, example:
>
> +	  /* We recognize four formats:
> +	     1. Two digits specify hours.
> +	     2. Four digits specify hours and minutes.
> +	     3. Two digits, ':', and two digits specify hours and minutes.
> +	     4. 'Z' is equivalent to +0000.  */

OK.

> Functionally OK, but needs to also check that the next character after
> rp is a digit so that we will accept input like "00:30" and reject input
> like "00::::30". Perhaps something like:
>
> +		if (*rp == ':' && n == 2 && isdigit(*(rp + 1)))
> +		  ++rp;

Done.

>>    i = sprintf (buf, "%s %c", dummy_string, sign);
>> -  snprintf (buf + i, ndigits + 1, "%04u", hhmm);
>> +  snprintf (buf + i,
>> +            (colon && ndigits >= 2) ? ndigits + 2 : ndigits + 1,
>> +            "%02u%s%02u", hh, colon ? ":" : "", mm);
>
> Functionally OK, but formatting issue.  The style in this file is to use
> tab for every 8 leading whitespace and the patch uses 8 spaces instead.
> indent -npro can help here.

I am using the "gnu" style in Emacs, but it doesn't seem to toggle to
tabs automatically. I fixed that.

> The logic above is OK when ndigits >= 3.  When ndigits < 3, there is no
> point to calling mkbuf with a colon parameter of true (it is the same
> test as setting it false).  Better would be:
>
> +
> + 	if (ndigits >= 3)
> + 	  {
> +	    expect = mkbuf (buf, false, true, hhmm, ndigits);
> +	    result |= compare (buf, expect, nresult);
> +
> +	    expect = mkbuf (buf, true, true, hhmm, ndigits);
> + 	    result |= compare (buf, expect, nresult);
> + 	  }
>         }

OK.

New patchset in a few minutes.
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index 490cfbef5025..177d80cad358 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@ 
 2015-09-04  Vincent Bernat  <vincent@bernat.im>
 
+	[BZ #17887]
+	* time/strptime_l.c (__strptime_internal): Make %z accept
+	[+-]HH:MM time zones.
+
+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.
diff --git a/time/strptime_l.c b/time/strptime_l.c
index 8fdd4e8e3f7a..07aabc8deb54 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 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))
@@ -769,6 +771,8 @@  __strptime_internal (rp, fmt, tmp, statep LOCALE_PARAM)
 	      {
 		val = val * 10 + *rp++ - '0';
 		++n;
+		if (*rp == ':' && n == 2)
+		  ++rp;
 	      }
 	    if (n == 2)
 	      val *= 100;
diff --git a/time/tst-strptime2.c b/time/tst-strptime2.c
index cfb7d70c4cc2..318fab973a36 100644
--- a/time/tst-strptime2.c
+++ b/time/tst-strptime2.c
@@ -34,7 +34,8 @@  static bool verbose;
    Note that a valid timezone string contains the following fields:
      Sign field consisting of a '+' or '-' sign,
      Hours field in two decimal digits, and
-     optional Minutes field in two decimal digits.
+     optional Minutes field in two decimal digits. Optionally,
+     a ':' is used to seperate hours and minutes.
 
    This function may write test strings with minutes values outside
    the valid range 00-59.  These are invalid strings and useful for
@@ -55,7 +56,7 @@  static bool verbose;
    range of 00 to 59.  */
 
 static long int
-mkbuf (char *buf, bool neg, unsigned int hhmm, size_t ndigits)
+mkbuf (char *buf, bool neg, bool colon, unsigned int hhmm, size_t ndigits)
 {
   const int mm_max = 59;
   char sign = neg ? '-' : '+';
@@ -65,7 +66,9 @@  mkbuf (char *buf, bool neg, unsigned int hhmm, size_t ndigits)
   long int expect = LONG_MAX;
 
   i = sprintf (buf, "%s %c", dummy_string, sign);
-  snprintf (buf + i, ndigits + 1, "%04u", hhmm);
+  snprintf (buf + i,
+            (colon && ndigits >= 2) ? ndigits + 2 : ndigits + 1,
+            "%02u%s%02u", hh, colon ? ":" : "", mm);
 
   if (mm <= mm_max && (ndigits == 2 || ndigits == 4))
     {
@@ -176,10 +179,18 @@  do_test (void)
       {
 	/* Test both positive and negative signs.  */
 
-	expect = mkbuf (buf, false, hhmm, ndigits);
+	expect = mkbuf (buf, false, false, hhmm, ndigits);
 	result |= compare (buf, expect, nresult);
 
-	expect = mkbuf (buf, true, hhmm, ndigits);
+	expect = mkbuf (buf, true, false, hhmm, ndigits);
+	result |= compare (buf, expect, nresult);
+
+        /* Test with colon as well.  */
+
+	expect = mkbuf (buf, false, true, hhmm, ndigits);
+	result |= compare (buf, expect, nresult);
+
+	expect = mkbuf (buf, true, true, hhmm, ndigits);
 	result |= compare (buf, expect, nresult);
       }