| Message ID | 0d0e4d8edce37dfef13e573588f0c043ddf07f6a.1587644889.git.congdanhqx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More ISO-8601 support | expand |
Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: > In a later patch, we will reuse this logic, move it to a helper, now. > > While we're at it, explicit states that we intentionally ignore "explicitly state", perhaps. > old-and-defective 2nd leap second. > > Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> > --- > date.c | 22 +++++++++++++++++----- > 1 file changed, 17 insertions(+), 5 deletions(-) > > diff --git a/date.c b/date.c > index b67c5abe24..f5d5a91208 100644 > --- a/date.c > +++ b/date.c > @@ -539,6 +539,22 @@ static int set_date(int year, int month, int day, struct tm *now_tm, time_t now, > return -1; > } > > +static int set_time(long hour, long minute, long second, struct tm *tm) > +{ > + /* C90 and old POSIX accepts 2 leap seconds, it's a defect, > + * ignore second number 61 > + */ /* * Style: our multi-line comments ought to be * formatted like this. Slash-asterisk that opens, * and asterisk-slash that closes, are both on their * own lines. */ But I am not sure we want to even have a new comment here. After all we are extracting/reinventing exactly the same logic as the original. Why we allow "60" might be worth commenting, but if a minute that has 62 seconds is a mere historical curiosity, then is it worth explaining why "61", which we never even wrote in the code, is missing from here? > + if (0 <= hour && hour <= 24 && > + 0 <= minute && minute < 60 && > + 0 <= second && second <= 60) { > + tm->tm_hour = hour; > + tm->tm_min = minute; > + tm->tm_sec = second; > + return 0; > + } > + return -1; > +} I am a bit surprised to see that you chose to unify with the "check and set" interface of is_date (now set_date). I was expecting to see that we'd have "check-only" helper functions. This is not a complaint, at least not yet until we see the result of using it in new code; it may very well be possible that the "check and set" interface would make the new caller(s) clearer. > static int match_multi_number(timestamp_t num, char c, const char *date, > char *end, struct tm *tm, time_t now) > { > @@ -556,12 +572,8 @@ static int match_multi_number(timestamp_t num, char c, const char *date, > case ':': > if (num3 < 0) > num3 = 0; > - if (num < 25 && num2 >= 0 && num2 < 60 && num3 >= 0 && num3 <= 60) { > - tm->tm_hour = num; > - tm->tm_min = num2; > - tm->tm_sec = num3; > + if (set_time(num, num2, num3, tm) == 0) > break; > - } > return 0; This caller does become easier to follow, I would say. Nicely done. > case '-':
On 2020-04-23 13:18:25-0700, Junio C Hamano <gitster@pobox.com> wrote: > Đoàn Trần Công Danh <congdanhqx@gmail.com> writes: > > > In a later patch, we will reuse this logic, move it to a helper, now. > > > > While we're at it, explicit states that we intentionally ignore > > "explicitly state", perhaps. > > > old-and-defective 2nd leap second. > > > > Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> > > --- > > date.c | 22 +++++++++++++++++----- > > 1 file changed, 17 insertions(+), 5 deletions(-) > > > > diff --git a/date.c b/date.c > > index b67c5abe24..f5d5a91208 100644 > > --- a/date.c > > +++ b/date.c > > @@ -539,6 +539,22 @@ static int set_date(int year, int month, int day, struct tm *now_tm, time_t now, > > return -1; > > } > > > > +static int set_time(long hour, long minute, long second, struct tm *tm) > > +{ > > + /* C90 and old POSIX accepts 2 leap seconds, it's a defect, > > + * ignore second number 61 > > + */ > > /* > * Style: our multi-line comments ought to be > * formatted like this. Slash-asterisk that opens, > * and asterisk-slash that closes, are both on their > * own lines. > */ > > But I am not sure we want to even have a new comment here. After > all we are extracting/reinventing exactly the same logic as the > original. Why we allow "60" might be worth commenting, but if a > minute that has 62 seconds is a mere historical curiosity, then is > it worth explaining why "61", which we never even wrote in the code, > is missing from here? I think single line like: /* We accept 61st second for the single? leap second */ Or something along the time, is good enough. Not sure if we want the word "single" there, though. I think majority of people don't even know about leap second. Probability that know about 62nd second is rarer, I think. > > + if (0 <= hour && hour <= 24 && > > + 0 <= minute && minute < 60 && > > + 0 <= second && second <= 60) { > > + tm->tm_hour = hour; > > + tm->tm_min = minute; > > + tm->tm_sec = second; > > + return 0; > > + } > > + return -1; > > +} > > I am a bit surprised to see that you chose to unify with the "check > and set" interface of is_date (now set_date). I was expecting to > see that we'd have "check-only" helper functions. > > This is not a complaint, at least not yet until we see the result of > using it in new code; it may very well be possible that the "check > and set" interface would make the new caller(s) clearer. > > > static int match_multi_number(timestamp_t num, char c, const char *date, > > char *end, struct tm *tm, time_t now) > > { > > @@ -556,12 +572,8 @@ static int match_multi_number(timestamp_t num, char c, const char *date, > > case ':': > > if (num3 < 0) > > num3 = 0; > > - if (num < 25 && num2 >= 0 && num2 < 60 && num3 >= 0 && num3 <= 60) { > > - tm->tm_hour = num; > > - tm->tm_min = num2; > > - tm->tm_sec = num3; > > + if (set_time(num, num2, num3, tm) == 0) > > break; > > - } > > return 0; > > This caller does become easier to follow, I would say. Nicely done. Yes, when I looked around date.c I saw that the only usecase for validate time is for setting it. And the incoming patch also has that usage. I chose to unify those code path to not buy me too much trouble. I'll take that "Nicely done" means this unification is OK for this series.
Danh Doan <congdanhqx@gmail.com> writes: > I think single line like: > > /* We accept 61st second for the single? leap second */ > > Or something along the time, is good enough. Not sure if we want the > word "single" there, though. I do not particularly want to see the single but without it, the single-one comment looks perfect. >> > - if (num < 25 && num2 >= 0 && num2 < 60 && num3 >= 0 && num3 <= 60) { >> > - tm->tm_hour = num; >> > - tm->tm_min = num2; >> > - tm->tm_sec = num3; >> > + if (set_time(num, num2, num3, tm) == 0) >> > break; >> > - } >> > return 0; >> >> This caller does become easier to follow, I would say. Nicely done. > > Yes, when I looked around date.c > > I saw that the only usecase for validate time is for setting it. > And the incoming patch also has that usage. > > I chose to unify those code path to not buy me too much trouble. > > I'll take that "Nicely done" means this unification is OK for this > series. The OK was meant for this single place that was converted, not any other place you'd use in the remainder of the series. And I think it was not such a good idea to use it twice, but I think with the suggested rewrite you took in v5, the other call site is also OK. Thanks.
diff --git a/date.c b/date.c index b67c5abe24..f5d5a91208 100644 --- a/date.c +++ b/date.c @@ -539,6 +539,22 @@ static int set_date(int year, int month, int day, struct tm *now_tm, time_t now, return -1; } +static int set_time(long hour, long minute, long second, struct tm *tm) +{ + /* C90 and old POSIX accepts 2 leap seconds, it's a defect, + * ignore second number 61 + */ + if (0 <= hour && hour <= 24 && + 0 <= minute && minute < 60 && + 0 <= second && second <= 60) { + tm->tm_hour = hour; + tm->tm_min = minute; + tm->tm_sec = second; + return 0; + } + return -1; +} + static int match_multi_number(timestamp_t num, char c, const char *date, char *end, struct tm *tm, time_t now) { @@ -556,12 +572,8 @@ static int match_multi_number(timestamp_t num, char c, const char *date, case ':': if (num3 < 0) num3 = 0; - if (num < 25 && num2 >= 0 && num2 < 60 && num3 >= 0 && num3 <= 60) { - tm->tm_hour = num; - tm->tm_min = num2; - tm->tm_sec = num3; + if (set_time(num, num2, num3, tm) == 0) break; - } return 0; case '-':
In a later patch, we will reuse this logic, move it to a helper, now. While we're at it, explicit states that we intentionally ignore old-and-defective 2nd leap second. Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> --- date.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)