[webkit-reviews] review denied: [Bug 32304] JSC::gregorianDateTimeToMS() returns negative values for huge years : [Attachment 44511] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied TAMURA, Kent <tkent at chromium.org>'s
request for review:
Bug 32304: JSC::gregorianDateTimeToMS() returns negative values for huge years
https://bugs.webkit.org/show_bug.cgi?id=32304

Attachment 44511: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=44511&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
This patch does not include a test case. Why not?

> -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.

> +    // 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.