Last Comment Bug 771744 - Various Number.prototype.* refactoring to better accommodate method-guarding work
: Various Number.prototype.* refactoring to better accommodate method-guarding ...
Status: RESOLVED FIXED
[js:t]
:
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: 795745
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-06 19:58 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-10-01 16:32 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove num_to and fold its work into its callers (6.49 KB, patch)
2012-07-06 19:58 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Move decls in num_toLocaleString closer to first uses (3.57 KB, patch)
2012-07-06 20:00 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Remove num_toStringHelper, moving and simplifying its code into callers (2.68 KB, patch)
2012-07-06 20:02 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:58:08 PDT
Created attachment 639910 [details] [diff] [review]
Remove num_to and fold its work into its callers

There might be something of a kernel of this to pull out into a single method, perhaps.  But the incredible length of num_to's parameter list made it really hard to understand a call to num_to.  And the argument-grokking parts are pretty much different with every function that needs them.  So it seems better, if not super-awesome, to be more elaborate here.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-06 20:00:42 PDT
Created attachment 639911 [details] [diff] [review]
Move decls in num_toLocaleString closer to first uses

It would have been folly to touch this function before the variable moving happened, so make those moves now and keep the more substantial refactoring in other patches.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-06 20:02:49 PDT
Created attachment 639912 [details] [diff] [review]
Remove num_toStringHelper, moving and simplifying its code into callers
Comment 3 Luke Wagner [:luke] 2012-07-07 04:44:50 PDT
Comment on attachment 639910 [details] [diff] [review]
Remove num_to and fold its work into its callers

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

This one is on the edge of too-much-inlining.  But just as with the last time, there is a healthy middle-road that factors out common building blocks.

::: js/src/jsnum.cpp
@@ +776,5 @@
> +            ToCStringBuf cbuf;
> +            char *numStr = IntToCString(&cbuf, int(precision));
> +            JS_ASSERT(numStr);
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_PRECISION_RANGE, numStr);
> +            return JS_FALSE;

The contents of this then-block are identical, please factor it out.

@@ +787,5 @@
> +    if (!numStr) {
> +        JS_ReportOutOfMemory(cx);
> +        return JS_FALSE;
> +    }
> +    JSString *str = js_NewStringCopyZ(cx, numStr);

from 'char buf' to here should also be factored out; the only difference is the parameters to js_dtostr which make just as much sense passing to the factored-out function.
Comment 4 Luke Wagner [:luke] 2012-07-07 04:45:31 PDT
(In reply to Luke Wagner [:luke] from comment #3)
Oh, I forgot, please s/JS_FALSE/false/
Comment 5 Luke Wagner [:luke] 2012-07-07 04:48:12 PDT
Comment on attachment 639912 [details] [diff] [review]
Remove num_toStringHelper, moving and simplifying its code into callers

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

::: js/src/jsnum.cpp
@@ +624,5 @@
> +    if (!str) {
> +        JS_ReportOutOfMemory(cx);
> +        return false;
> +    }
> +    args.rval().setString(str);

Could you post-pone callee-clobbering to the end of the native?

@@ +721,4 @@
>      }
>  
>      if (cx->localeCallbacks && cx->localeCallbacks->localeToUnicode) {
> +        JSBool ok = cx->localeCallbacks->localeToUnicode(cx, buf, &args.rval());

s/JSBool/bool/
Comment 8 :Ms2ger (⌚ UTC+1/+2) 2012-07-11 12:05:49 PDT
Comment on attachment 639912 [details] [diff] [review]
Remove num_toStringHelper, moving and simplifying its code into callers

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

::: js/src/jsnum.cpp
@@ +621,5 @@
> +        return ok;
> +
> +    Rooted<JSString*> str(cx, js_NumberToStringWithBase(cx, d, 10));
> +    if (!str) {
> +        JS_ReportOutOfMemory(cx);

js_NumberToStringWithBase already seems to report OOM.
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2012-07-13 23:49:25 PDT
Actually, if I remember right, it's inconsistent.  :-\  Here I decided to just leave it alone and screwy for now -- really should have filed a followup bug on it, I guess.  Filed bug 773922 for that.

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