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

Make ToNumber(String) conversion more spec-compliant #383

Merged
merged 1 commit into from
Jan 31, 2018

Conversation

sainaen
Copy link
Contributor

@sainaen sainaen commented Jan 19, 2018

Update ToNumber conversion applied to the String type:

  • plus (+) or minus (-) signs are not allowed before the hex literals,
    return NaN for them
  • also return NaN for HexIntegralLiteral followed by anything other than
    whitespace (in contrast to how 'parseInt()' works)
  • add support for binary ('0b101010') and octal ('0o52') literals

New behavior is applied only if the language version is >=ES6, so it
shouldn’t brake any existing code.

This also enables built-ins/Number test262 cases.


Fixes #368.

I’ve also verified that the new ToNumberLegacyConversionsTest works in the master branch without this patch, so it in fact represents the ToNumber's current behavior.

@sainaen
Copy link
Contributor Author

sainaen commented Jan 19, 2018

I have one question related to this.

I decided to not change the tests that cover some non-spec’ed functionality in MozSuite, which leaves it on the language version < ES6. Is it ok?
I think in SpiderMonkey they slowly remove parts of their own tests that are covered by test262, maybe that's something we should consider too?

Copy link
Collaborator

@gbrail gbrail left a comment

Choose a reason for hiding this comment

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

Looks good to me -- I understand what you did to this complex code and I see that it's tested and works for my simple tests as well.

Curious if anyone else has also inspected this.

I do think it's safest to leave this to ">=ES6" for now since backward compatibility can show up at the worst time!

Update ToNumber conversion applied to the String type:
* plus (+) or minus (-) signs are not allowed before the hex literals,
  return NaN for them
* also return NaN for HexIntegralLiteral followed by anything other than
  whitespace (in contrast to how 'parseInt()' works)
* add support for binary ('0b101010') and octal ('0o52') literals

New behavior is applied only if the language version is >=ES6, so it
shouldn't brake any existing code.

This also enables built-ins/Number test262 cases.
@gbrail gbrail merged commit 9641e58 into mozilla:master Jan 31, 2018
@sainaen sainaen deleted the fix-toNumber branch January 31, 2018 19:29
@sainaen
Copy link
Contributor Author

sainaen commented Jan 31, 2018

Thanks for merging!

I do think it's safest to leave this to ">=ES6" for now since backward compatibility can show up at the worst time!

Sure, let’s leave it for now.

It’s just that I’m a bit concerned it may become harder to update in future if we drift away too much. Maybe create an issue to keep track of it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants