Number.{parseInt, parseFloat} should be the same functions as global ones

RESOLVED FIXED in Firefox 39

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ziyunfei, Assigned: ziyunfei)

Tracking

(Blocks: 1 bug)

Trunk
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
js> Number.parseInt == parseInt
false // should be true
js> Number.parseFloat == parseFloat
false // should be true
(Assignee)

Comment 1

3 years ago
Created attachment 8568400 [details] [diff] [review]
bug-1135560-v1.patch
Assignee: nobody → 446240525
Attachment #8568400 - Flags: review?(till)
Comment on attachment 8568400 [details] [diff] [review]
bug-1135560-v1.patch

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

Great! f+ instead of r+ because I'd like to see a second version with the comment below addressed.

::: js/src/jsnum.cpp
@@ +1194,5 @@
>          return nullptr;
>      if (!JS_DefineFunctions(cx, global, number_functions))
>          return nullptr;
> +    /* Number.parseInt should be the same function object as global parseInt. */
> +    JSFunction *parseInt = JS_DefineFunction(cx, global, "parseInt", num_parseInt, 2, 0);

I think it'd be better to use the non-jsapi versions of JS_DefineFunction and JS_DefineProperty here. That way we don't have to atomize the names at all: they're already in the commonNames list. To do this, get the id like so:
RootedId id(cx, NameToId(cx->runtime()->commonNames->parseInt));

For DefineProperty, pass nullptr for the get and set op parameters.
Attachment #8568400 - Flags: review?(till) → feedback+
(Assignee)

Comment 3

3 years ago
Created attachment 8568534 [details] [diff] [review]
bug-1135560-v2.patch
Attachment #8568400 - Attachment is obsolete: true
Attachment #8568534 - Flags: review?(till)
Comment on attachment 8568534 [details] [diff] [review]
bug-1135560-v2.patch

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

Excellent!
Attachment #8568534 - Flags: review?(till) → review+
https://hg.mozilla.org/mozilla-central/rev/617d8a7292f1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.