Closed
Bug 1160356
Opened 9 years ago
Closed 9 years ago
Various Date bug-fixes, in pursuit of making DateObject::setUTCTime assert TimeClip'd-ness
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: Waldo, Assigned: Waldo)
Details
Attachments
(3 files)
4.24 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
Reorganize Date function/constructor code into three separate methods, aligning with ES6 definitions
4.34 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
7.88 KB,
patch
|
evilpie
:
review+
|
Details | Diff | Splinter Review |
The goal here is to help clarify that a few places that set the time in a Date object, do so with a clipped time. But incidentally, we fix a few longstanding bugs wrt the spec. Win-win!
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8600128 -
Flags: review?(evilpies)
Assignee | ||
Comment 2•9 years ago
|
||
No algorithm/code changes, just code motion. Yes, technically the not-constructing cases could be commoned as before. But I'd really rather not, to parallel the spec better, for clarity.
Attachment #8600133 -
Flags: review?(evilpies)
Assignee | ||
Comment 3•9 years ago
|
||
Sure, the old date_msecFromArgs method commoned up a little code. I'd argue, given the specs don't common this code, it hurts readability more than it helps. Oh, if you looked closely and saw that date_msecFromArgs did ToInteger in addition to ToNumber but this new code doesn't -- MakeTime/MakeDate ToInteger all their arguments, so the old code was doing semi-wasted work. New code aligns with specs *and* is faster -- again win!
Attachment #8600137 -
Flags: review?(evilpies)
Updated•9 years ago
|
Attachment #8600128 -
Flags: review?(evilpies) → review+
Comment 4•9 years ago
|
||
Comment on attachment 8600133 [details] [diff] [review] Reorganize Date function/constructor code into three separate methods, aligning with ES6 definitions Review of attachment 8600133 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsdate.cpp @@ +3047,5 @@ > +{ > + MOZ_ASSERT(args.length() == 1); > + > + if (args.isConstructing()) { > + double d; This doesn't look like the ES6 steps at all.. :/
Attachment #8600133 -
Flags: review?(evilpies) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8600137 [details] [diff] [review] Make new Date(arg1, arg2, ...) conform to ES3-6 in converting *all* arguments to number before computing the return value Review of attachment 8600137 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsdate.cpp @@ -578,5 @@ > -#define MAXARGS 7 > - > -static bool > -date_msecFromArgs(JSContext* cx, CallArgs args, double* rval) > -{ Thanks for removing this. @@ +3034,4 @@ > return ToDateString(cx, args, NowAsMillis()); > } > > static bool // ES6 20.3.2.1 @@ +3043,2 @@ > if (args.isConstructing()) { > + // Steps 3a-b. Please use 3.a-b. here and similar everywhere else.
Attachment #8600137 -
Flags: review?(evilpies) → review+
Comment 6•9 years ago
|
||
Please also fix the BUGNUMBER in your tests and add the spec chapter of DateOneArgument/DateNoArgument.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9804500b0ab7 https://hg.mozilla.org/integration/mozilla-inbound/rev/a411bd3307da https://hg.mozilla.org/integration/mozilla-inbound/rev/f35d190d8e2f
Assignee | ||
Comment 8•9 years ago
|
||
Fixed up the bug numbers, didn't "fix" the step numbers because .cpp files were almost uniform the other way. Did *not* add step numbers or spec section comments, because these algorithms aren't the ES6 ones, and having them be ES6 requires NewTarget which we don't have yet. Fixing the algorithms, in general, to be ES6-compatible seems best left to subsequent work.
Comment 9•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9804500b0ab7 https://hg.mozilla.org/mozilla-central/rev/a411bd3307da https://hg.mozilla.org/mozilla-central/rev/f35d190d8e2f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•