[2/2] time: in strptime(), make %z accept [+-]HH:MM tz [BZ #17887]
Commit Message
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
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
❦ 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.
@@ -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.
@@ -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;
@@ -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);
}