Closed
Bug 771744
Opened 12 years ago
Closed 12 years ago
Various Number.prototype.* refactoring to better accommodate method-guarding work
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: Waldo, Assigned: Waldo)
References
Details
(Whiteboard: [js:t])
Attachments
(3 files)
6.49 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
3.57 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
2.68 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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)
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #639912 -
Flags: review?(luke)
Comment 3•12 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•12 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #3) Oh, I forgot, please s/JS_FALSE/false/
Updated•12 years ago
|
Attachment #639911 -
Flags: review?(luke) → review+
Comment 5•12 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+
Updated•12 years ago
|
Whiteboard: [js:t]
Assignee | ||
Comment 6•12 years ago
|
||
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
Comment 7•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Comment 8•12 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 @@ +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.
Assignee | ||
Comment 9•12 years ago
|
||
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.
Description
•