Closed
Bug 1406993
Opened 7 years ago
Closed 7 years ago
Pick newer equivalent years when the year >= 2038
Categories
(Core :: JavaScript: Standard Library, enhancement, P3)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: anba, Assigned: anba)
References
Details
Attachments
(1 file, 2 obsolete files)
11.83 KB,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
{
{2034, 2035, 2030, 2031, 2037, 2027, 2033},
{2012, 2024, 2036, 2020, 2032, 2016, 2028}
};
are probably better choices for equivalent years after 2037 than:
{
{1978, 1973, 1974, 1975, 1981, 1971, 1977},
{1984, 1996, 1980, 1992, 1976, 1988, 1972}
};
Assignee | ||
Comment 1•7 years ago
|
||
It seems to more useful to use current resp. near future years for DST computation of years after 2037 than past years. And it also fixes another issue where the time zone offset and time zone ID in Date.prototype.toString() don't match:
Before the patch: |new Date(2038, 11-1, 1).toString()| returns "Mon Nov 01 2038 00:00:00 GMT-0800 (PDT)".
With the patch: |new Date(2038, 11-1, 1).toString()| returns "Mon Nov 01 2038 00:00:00 GMT-0700 (PDT)".
Attachment #8916769 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 2•7 years ago
|
||
Linux(arm) doesn't get this right [1]. I guess I'll just change ToPRMJTime to call EquivalentYearForDST, too.
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc6a24ec10173c5f262c52d57fecaf874013911d
Updated•7 years ago
|
Attachment #8916769 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 3•7 years ago
|
||
Requesting a new review, because too much changed compared to the first version.
The original patch failed on Linux(arm) because "Mar 31 2040 00:00:00" cannot be represented as a 32-bit time_t value. mktime returned -1 to notify the input can't be processed properly, but since we didn't perform any error handling at all for mktime (and localtime_r) [1], we got bogus results.
The new patch checks the return values for mktime and localtime_r and tries to workaround 32-bit time_t limitations if possible:
- If the initial mktime failed, we retry it with an equivalent year.
- If it still fails or localtime_r failed, we simply use a fall back value for the time zone offset and set tm_zone to the empty string [2].
I've also changed the start time when we apply the equivalent year workaround to include midnight (start of day) 01/01/2038. I found it confusing not to include the start of the day, well even more confusing is the value of MaxUnixTimeT in [3], which is 12/31/2037 00:00 am, but PST time, but that's another issue. :-)
[1] https://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/js/src/vm/Time.cpp#310-311
[2] tm::tm_zone isn't a "const char*" on all platforms, so we can't directly assign the empty string literal to tm_zone, but instead need a local "char[]": https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed68be17d6d94cc3c1c97a099438741e99ce4d15
[3] https://searchfox.org/mozilla-central/rev/b53e29293c9e9a2905f4849f4e3c415e2013f0cb/js/src/vm/DateTime.h#176
Attachment #8916769 -
Attachment is obsolete: true
Attachment #8917013 -
Flags: review?(jwalden+bmo)
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 4•7 years ago
|
||
Note to self: Remember testing bug 1029923 after the change has landed.
Comment 5•7 years ago
|
||
Comment on attachment 8917013 [details] [diff] [review]
bug1406993.patch
Review of attachment 8917013 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsdate.cpp
@@ +445,5 @@
> int day = int(DayFromYear(year) + 4) % 7;
> if (day < 0)
> day += 7;
>
> + auto& yearStartingWith = year < 1970 ? pastYearStartingWith : futureYearStartingWith;
Tack a const on that
@@ +450,5 @@
> return yearStartingWith[IsLeapYear(year)][day];
> }
>
> +// Return true if |t| is representable as a 32-bit time_t variable, that means
> +// the year is in [1970, 2038[.
We tend to use () for exclusive bounds in comments (or at least more often than this), so [1970, 2038).
@@ +454,5 @@
> +// the year is in [1970, 2038[.
> +static bool
> +IsRepresentableAsTime32(double t)
> +{
> + return t >= 0.0 && t < 2145916800000.0;
Mild preference for |0.0 <= t && t < ...| so it looks more like a three-part inequality.
::: js/src/vm/Time.cpp
@@ +267,5 @@
> size_t result = 0;
> #if defined(XP_UNIX) || defined(XP_WIN)
> struct tm a;
> int fake_tm_year = 0;
> + char emptyTimeZoneId[] = "";
Move this next to the one use of it, or at least as close to it as possible for the struct it gets put in? Enough of this C hangover. :-)
@@ +308,5 @@
> td.tm_wday = prtm->tm_wday;
> td.tm_year = prtm->tm_year - 1900;
> td.tm_yday = prtm->tm_yday;
> td.tm_isdst = prtm->tm_isdst;
> time_t t = mktime(&td);
Add a blank line before this.
@@ +340,5 @@
> * in that case).
> * e.g. new Date(1873, 0).toLocaleFormat('%Y %y') => "1873 73"
> * See bug 327869.
> */
> + static const int FAKE_YEAR_BASE = 9900;
Maybe s/static const/constexpr/?
Attachment #8917013 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 6•7 years ago
|
||
Applied review comments, carrying r+.
Attachment #8917013 -
Attachment is obsolete: true
Attachment #8924998 -
Flags: review+
Assignee | ||
Comment 7•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c5fbdd2af8e45c5fafd8af65aa05d23f566f3c7
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45a2ee7d468e
Add a new equivalent year mapping table for years after 2037. r=Waldo
Keywords: checkin-needed
![]() |
||
Comment 9•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•