771742 - Refactor many of the Date methods to be implemented in separate methods (not in single utility functions), using the spec algorithms

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

The current NonGenericMethodGuard idiom is awkward and doesn't read very naturally.  It asks for a JSClass* even when the method in question might need to accept objects (this values, more generally) that are of multiple classes.  It also results in weird function-pointer-passing behaviors that aren't easy to use, or to understand on the implementation side.

We're introducing a new system to address this, which will take a test function (that says if a value is an acceptable this) and an implementation function (which will perform the implemented function's semantics, knowing that the this value passed the test).  To do this we need to refactor some of the existing method-guard code, which currently has to pass along the function pointer for recursive invocation, and which can't really be adapted to the new form.  This bug will hold patches to do this refactoring.  (The actual method guard idiom change more generally will be a separate bug.)

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Preliminary reorganization of various Date spec methods/constants

The existing Date.prototype.* code makes a bunch of assumptions about the exact way we implement stuff now -- that NaNs won't enter, that if they do they'll propagate nicely, and so on.  So there's a little more work being done in this patch than in the code before it.  But it's very little -- think more isNaN or ToInteger computations -- and should be negligible overall, and it'll allow us to implement the spec algorithms as the spec states them.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Implement the Date.prototype.set<time component> methods in spec terms, without using date_makeTime

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Same thing for Date.prototype.set<day/month/year duration>, without date_makeDate

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Date.prototype.<to locale-specific string> refactoring, also removing date_toLocale{String,}Helper

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Date.prototype.to{ISO,GMT}String refactoring

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 639901 [details] [diff] [review]
Preliminary reorganization of various Date spec methods/constants

Review of attachment 639901 [details] [diff] [review]:
-----------------------------------------------------------------

Nice

::: js/src/jsdate.cpp
@@ -189,5 @@
> -/* math here has to be f.p, because we need
> - *  floor((1968 - 1969) / 4) == -1
> - */
> -#define DayFromYear(y)  (365 * ((y)-1970) + floor(((y)-1969)/4.0)            \
> -                         - floor(((y)-1901)/100.0) + floor(((y)-1601)/400.0))

But it was so readable before!

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 639902 [details] [diff] [review]
Implement the Date.prototype.set<time component> methods in spec terms, without using date_makeTime

Mmmm, Waldo-style spec comments; good stuff.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 639903 [details] [diff] [review]
Same thing for Date.prototype.set<day/month/year duration>, without date_makeDate

Review of attachment 639903 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsdate.cpp
@@ -2127,5 @@
> -        SetDateToNaN(cx, thisObj, &args.rval());
> -        return true;
> -    }
> -
> -    unsigned numNums = Min(args.length(), maxargs);

I'm gonna miss numNums.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 639907 [details] [diff] [review]
Date.prototype.to{ISO,GMT}String refactoring

No /* Step 1 */ comments?

Comment 10

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5493f54ce621
https://hg.mozilla.org/integration/mozilla-inbound/rev/65396577e80a
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68d62b3ab83
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f326e125cda
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3382b5975c8

toISOString prescribes a result but doesn't specify an algorithm to produce it, so it has no steps.  toUTCString is specified to produce an implementation-dependent result and similarly has no steps.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5493f54ce621
https://hg.mozilla.org/mozilla-central/rev/65396577e80a
https://hg.mozilla.org/mozilla-central/rev/e68d62b3ab83
https://hg.mozilla.org/mozilla-central/rev/6f326e125cda
https://hg.mozilla.org/mozilla-central/rev/c3382b5975c8