Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A few small changes to NativeNumber #883

Merged
merged 2 commits into from
May 7, 2021

Conversation

tonygermano
Copy link
Contributor

  • Implement Number.EPSILON
  • Number.parseFloat and Number.parseInt use the same built-in function objects that are the initial value of the "parseFloat" and "parseInt" properties of the global object.

((IdFunctionObject) parseInt).addAsProperty(ctor);
} else {
addIdFunctionProperty(ctor, NUMBER_TAG, ConstructorId_parseInt, "parseInt", 1);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these checks needed or can we assume that parseInt and parseFloat are always set in the top level scope?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the check is needed - i think we can assume the objects are there; or we can add an assert.

While looking at the NativeGlobal code it looks like the arity of parseInt is 2 for native. Any idea?

case Id_parseInt:
            name = "parseInt";
            arity = 2;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think "parseInt" and "parseFloat" would be missing but there are so many other weird things that can happen in JavaScript.

Since the spec does seem to insist that they are literally the same function, why don't you just leave in the "if" and not the "else". That way it'll always work even if someone creates a weird top-level scope.

I'm also not sure what the "else" is going here -- does it result in a function that actually works?

Finally, parseInt is actually specified to take two arguments...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I just used the line that was already there, but it should be 2, as the second parameter is the radix. When it pulls the global function that line is skipped, and it correctly shows Number.parseInt.lenght == 2. The spec literally says:

21.1.2.13 Number.parseInt ( string, radix )
The value of the Number.parseInt data property is the same built-in function object that is the initial value of the "parseInt" property of the global object defined in 19.2.5.

I put the check in because I didn't know if it was possible to have a top level scope that didn't have all of the global object properties defined, and I didn't want to leave Number without these two methods if that was the case. Maybe it is not a concern?

It looks like the only place that the init methods for NativeGlobal and NativeNumber are called is in ScriptRuntime.initSafeStandardObjects. That method is called by Context.initSafeStandardObjects, and the javadoc says:

This method must be called to initialize a scope before scripts can be evaluated in that scope.

I guess if someone goes through all of the trouble to create a top level scope without calling that method, then they probably don't really need Number.parseInt or Number.parseFloat either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gbrail the else is calling the existing code that creates a separate IdFunctionObject on the Number built-in, which calls the same NativeGlobal implementation under the hood. Theoretically it would leave a working function in place, since it already does so without relying on the global object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took out the part that added the functions anyway if they were missing from the top level scope.

@tonygermano
Copy link
Contributor Author

I think this one is ready unless there are still unaddressed concerns.

@gbrail
Copy link
Collaborator

gbrail commented May 7, 2021

This one looks good too -- thanks!

@gbrail gbrail merged commit 7f53ff9 into mozilla:master May 7, 2021
@tonygermano tonygermano deleted the number-epsilon branch May 11, 2021 23:55
@p-bakker p-bakker added the feature Issues considered a new feature label Oct 13, 2021
@p-bakker p-bakker added this to the Release 1.7.14 milestone Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Issues considered a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants