Summary: | JSC::gregorianDateTimeToMS() returns negative values for huge years | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kent Tamura <tkent> | ||||||||||
Component: | Web Template Framework | Assignee: | Kent Tamura <tkent> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, darin, ggaren, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Kent Tamura
2009-12-08 21:40:24 PST
Oh, it's JSC::, not WTF::. Created attachment 44511 [details]
Proposed patch
style-queue ran check-webkit-style on attachment 44511 [details] without any errors.
Comment on attachment 44511 [details] Proposed patch 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. Created attachment 44611 [details] Proposed patch (rev.2) (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. style-queue ran check-webkit-style on attachment 44611 [details] without any errors.
Please test SunSpider with and without your patch, and post results. Created attachment 44652 [details] sunspider-compare-results output > Please test SunSpider with and without your patch, and post results. Here it is. The baseline is r51974 without this patch, and the competitor is r51974 + this patch. It seems there are no significant differences. Comment on attachment 44611 [details]
Proposed patch (rev.2)
Rejecting patch 44611 from commit-queue.
Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11764 test cases.
fast/js/date-daysfrom1970-overflow.html -> failed
Exiting early after 1 failures. 7083 tests run.
113.75s total testing time
7082 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
2 test cases (<1%) had stderr output
> fast/js/date-daysfrom1970-overflow.html -> failed
The test depends on the local timezone. I'll fix it.
Created attachment 44655 [details]
Proposed patch (rev.3)
* Fix a timezone-dependency of the test
style-queue ran check-webkit-style on attachment 44655 [details] without any errors.
Comment on attachment 44655 [details] Proposed patch (rev.3) Clearing flags on attachment: 44655 Committed r51986: <http://trac.webkit.org/changeset/51986> All reviewed patches have been landed. Closing bug. |