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

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(3 attachments)

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.
Attachment #639910 - Flags: review?(luke)
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.
Attachment #639911 - Flags: review?(luke)
Created attachment 639912 [details] [diff] [review]
Remove num_toStringHelper, moving and simplifying its code into callers
Attachment #639912 - Flags: review?(luke)

Comment 3

5 years ago
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+

Comment 4

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

Updated

5 years ago
Attachment #639911 - Flags: review?(luke) → review+

Comment 5

5 years ago
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]
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
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/4483ab804f1e
https://hg.mozilla.org/mozilla-central/rev/e0f23682ce95
https://hg.mozilla.org/mozilla-central/rev/7820ae26bd7f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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.
Depends on: 795745
You need to log in before you can comment on or make changes to this bug.