Closed Bug 771744 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

(Whiteboard: [js:t])

Attachments

(3 files)

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.
Attachment #639910 - Flags: review?(luke)
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.
Attachment #639911 - Flags: review?(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.
Attachment #639910 - Flags: review?(luke) → review+
(In reply to Luke Wagner [:luke] from comment #3)
Oh, I forgot, please s/JS_FALSE/false/
Attachment #639911 - Flags: review?(luke) → review+
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/
Attachment #639912 - Flags: review?(luke) → review+
Whiteboard: [js:t]
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.
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.
You need to log in before you can comment on or make changes to this bug.