771744 - Various Number.prototype.* refactoring to better accommodate method-guarding work

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: 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]]

Attachment: 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]]

Attachment: Remove num_toStringHelper, moving and simplifying its code into callers

Comment 3

[Luke Wagner [:luke]]

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]]

(In reply to Luke Wagner [:luke] from comment #3)
Oh, I forgot, please s/JS_FALSE/false/

Comment 5

[Luke Wagner [:luke]]

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 6

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4483ab804f1e
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f23682ce95
https://hg.mozilla.org/integration/mozilla-inbound/rev/7820ae26bd7f

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4483ab804f1e
https://hg.mozilla.org/mozilla-central/rev/e0f23682ce95
https://hg.mozilla.org/mozilla-central/rev/7820ae26bd7f

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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]]

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.