Thanks for taking this on. Here is a brief review.
The most important thing is that the patch also needs
to update doc/parse-datetime.texi. Also, some comments
about the code changes:
On 03/30/13 12:18, Mihai Capotă wrote:
> + /* not ISO 8601 time, forcing mktime error */
> + pc->hour = 90;
How does this force a mktime error? mktime allows tm_hour == 90.
> datetime:
> iso_8601_datetime
> + | iso_8601_basic_datetime
> ;
>
> iso_8601_datetime:
> iso_8601_date 'T' iso_8601_time
> ;
>
> +iso_8601_basic_datetime:
> + number 'T' iso_8601_basic_time
> + { pc->dates_seen--; } /* already incremented in digits_to_date_time */
This doesn't look right. 'number' accepts all sort of things that we
would rather not accept here. Conversely, why require ":" in times to
correlate with "-" in dates? Shouldn't we accept a "-"less date along
with a ":"ful time, and vice versa? And that "dates_seen--" business
is a hack; can't we arrange things so that dates_seen is incremented
just once?
> +iso_8601_basic_time:
> + tUNUMBER o_zone_offset
> + {
> + set_hhmmss_iso_8601_basic_time (pc, $1.value, 0);
> + pc->meridian = MER24;
> + }
> + | tUDECIMAL_NUMBER o_zone_offset
> + {
> + /* FIXME avoid time_t to long int cast */
Why is the cast needed? Also, can't the grammar be simplified
here, by using unsigned_seconds instead of using both
tUDECIMAL_NUMBER and tUNUMBER?
