Re: [PATCH v3 1/2] date.c: skip fractional second part of ISO-8601

> Danh Doan <congdanhqx@gmail.com> writes:
>
>>>>  		if (num < 25 && num2 >= 0 && num2 < 60 && num3 >= 0 && num3 <= 60) {
>>>>  			tm->tm_hour = num;
>>>>  			tm->tm_min = num2;
>>> And after all that is done, if <num2> (and others) are within a
>>> reasonable range, we use that as HH:MM:SS.  
>>>
>>> OK.  If <num2> (or <num3>, or even <num> for that matter) weren't
>>> reasonable, is it still sensible to discard the fractional part?
>>> The answer is not immediately obvious to me.
>>>
>>> To be safe, it might make sense to extract a helper function from
>>> the next conditional, i.e.
>>>
>>> static int is_hms(long num1, long num2, long num3)
>> I'll make it `is_time` on par with is_date check.
> That is probably a lot more readable name than is_hms().  

>
> I do not worry too much if the name is not "on par with" it, though,
> because is_date() does more than just "check", as you noticed below,
> unlike the "is hour-minute-seconds are within reasonable range?"
> check.
>
>> I'll look into it and check if int or long is suitable for parameter's
>> type.
>>
>>> {
>>> 	return (0 <= num1 && num1 < 25 &&
>>> 		0 <= num2 && num2 < 60 &&
>>> 		0 <= num3 && num3 <= 60);
>> Does it worth to add an explicit comment that we intentionally ignore
>> the old-and-defective 2nd leap seconds (i.e. second with value 61)?
>>
>> I saw in one of your previous email doubting if we should check for
>> `num3 <= 61` or not.
> I wrote that without checking anything, even what our own code does.
> As the existing code seems to want to work only with a second part
> that is 60 or less, not allowing a minute with 62 seconds, I think
> sticking to that and saying "0 <= num3 && num3 <= 60" is good.
>
>>> }
>>>
>>> and use it in the new "else if" block like so?
>>>
>>>
>>> 	} else if (*end == '.' && isdigit(end[1]) &&
>>> 		   is_date(tm->tm_year, tm->tm_mon, tm->tm_mday, NULL, now, tm) &&
>> When running into this, the code patch for non-approxidate hasn't
>> initialised value for now, yet.
> We may want to separate the logic that relies on the value of 'now'
> and 'now_tm->tm_year' out of is_date() to make it more easily
> reusable.  In this generic codepath, for example, we do not
> necessarily want to say "we refuse to parse timestamp that is 10
> days or more into the future".
>
> The longer I stare at is_date(), the more I am inclined to say it is
> a bit of impedance mismatch, and we instead should have the is_hms()
> helper as I suggested in the message you are responding to, plus
> something like:

>
> static int is_ymd(int year, int month, int day)
> {
>         /* like tm_to_time_t(), we only work between 1970-2099 */
> 	static const int days_in_month[] = {
> 		31, 28, 31, 30, ..., 31
> 	};
>
> 	return !(year < 1970 || 2100 <= year ||
> 		month <= 0 || 13 <= month ||
> 		day <= 0 || year / 4 + days_in_month[month-1] <= day);
> }
>
> and use it here.  I am not sure if we can reuse this inside
> is_date(), but if we can do so that would be good (and if we cannot,
> that is fine, too).
>
> Thanks.