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

[Vincent Bernat <Vincent dot Bernat at exoscale dot ch>]

 â 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.
-- 
Vincent Bernat â Vincent.Bernat@exoscale.ch
ââ http://www.exoscale.ch