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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(3 files)

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!
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)
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)
Attachment #8600128 - Flags: review?(evilpies) → review+
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 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+
Please also fix the BUGNUMBER in your tests and add the spec chapter of DateOneArgument/DateNoArgument.
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.
You need to log in before you can comment on or make changes to this bug.