[webkit-reviews] review requested: [Bug 32304] JSC::gregorianDateTimeToMS() returns negative values for huge years : [Attachment 44611] Proposed patch (rev.2)

[bugzilla-daemon at webkit.org]

TAMURA, Kent <tkent at chromium.org> has asked  for review:
Bug 32304: JSC::gregorianDateTimeToMS() returns negative values for huge years
https://bugs.webkit.org/show_bug.cgi?id=32304

Attachment 44611: Proposed patch (rev.2)
https://bugs.webkit.org/attachment.cgi?id=44611&action=review

------- Additional Comments from TAMURA, Kent <tkent at chromium.org>
(In reply to comment #4)
> (From update of attachment 44511 [details])
> This patch does not include a test case. Why not?

I added a test to the updated patch.
If 'yearday' in dateToDayInYear() is int, the test causes an assertion failure.


> > -static int dateToDayInYear(int year, int month, int day)
> > +static double dateToDayInYear(int year, int month, int day)
> 
> This seems wrong. A "day in year" value can never be out of range for an int:

> There are a maximum of 366 days in a year. So I don't see how the change in
> return type is helpful unless there is a bug inside the dateToDayInYear
> function. You'll have to explain why changing to double is helpful.

This function returns the number of days from 1970-01-01 to the specified date.
You know, (year - 1970) * 365 can be larger than INT_MAX.
The function name is very wrong. I renamed it in the updated patch.

> > +	 // 8.64E15 is a value near to the maximum integral number that
> > +	 // double can represent without precision loss.
> >	 if (fabs(t) > 8.64E15)
> 
> That comment seems to me like it's probably right right, but the best way to
> document such things is typically to use a named constant. Also, I believe
this
> number comes straight from the ECMAScript specification, and should not be
> changed. The comment makes it seem like you could come up with a value even
> closer to the maximum number, and improve the code, but I believe that is not

> true and would cause us to differ from the specification if we changed it.

ok, I introduced a named constant.