Last Comment Bug 771742 - Refactor many of the Date methods to be implemented in separate methods (not in single utility functions), using the spec algorithms
: Refactor many of the Date methods to be implemented in separate methods (not ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 771946
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-06 19:21 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-07-08 14:13 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Preliminary reorganization of various Date spec methods/constants (11.73 KB, patch)
2012-07-06 19:28 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Implement the Date.prototype.set<time component> methods in spec terms, without using date_makeTime (12.11 KB, patch)
2012-07-06 19:32 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Same thing for Date.prototype.set<day/month/year duration>, without date_makeDate (11.05 KB, patch)
2012-07-06 19:36 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Date.prototype.<to locale-specific string> refactoring, also removing date_toLocale{String,}Helper (4.59 KB, patch)
2012-07-06 19:39 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Date.prototype.to{ISO,GMT}String refactoring (2.79 KB, patch)
2012-07-06 19:43 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-06 19:21:19 PDT
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] (remove +bmo to email) 2012-07-06 19:28:00 PDT
Created attachment 639901 [details] [diff] [review]
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] (remove +bmo to email) 2012-07-06 19:32:49 PDT
Created attachment 639902 [details] [diff] [review]
Implement the Date.prototype.set<time component> methods in spec terms, without using date_makeTime
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-06 19:36:30 PDT
Created attachment 639903 [details] [diff] [review]
Same thing for Date.prototype.set<day/month/year duration>, without date_makeDate
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-06 19:39:41 PDT
Created attachment 639904 [details] [diff] [review]
Date.prototype.<to locale-specific string> refactoring, also removing date_toLocale{String,}Helper
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-06 19:43:37 PDT
Created attachment 639907 [details] [diff] [review]
Date.prototype.to{ISO,GMT}String refactoring
Comment 6 Luke Wagner [:luke] 2012-07-07 04:52:16 PDT
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] 2012-07-07 04:54:37 PDT
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] 2012-07-07 04:57:04 PDT
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] 2012-07-07 04:58:57 PDT
Comment on attachment 639907 [details] [diff] [review]
Date.prototype.to{ISO,GMT}String refactoring

No /* Step 1 */ comments?
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-07 21:48:28 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.