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

Fix integer parsing #1614

Merged
merged 3 commits into from
Oct 2, 2021
Merged

Fix integer parsing #1614

merged 3 commits into from
Oct 2, 2021

Conversation

nrabulinski
Copy link
Contributor

@nrabulinski nrabulinski commented Oct 2, 2021

This Pull Request makes it possible to continue with #1602 and also fixes some errors with parsing integers.

It changes the following:

  • Instead of manually parsing and adding each character I use i128::from_str_radix, which is then converted to f64

Unanswered questions:

- What does the spec say about even larger numbers? I've tested NodeJS and Firefox and you can more or less get unlimited range, albeit anything that's larger than Number.MAX_SAFE_INTEGER is not represented exactly. With this solution, however, numbers get clamped to -1.7e+38 and 1.7e+38. I'm assuming fixing this would most likely require an overhaul of the number representation and parsing code.

Instead of manually parsing and adding each character to i128::from_str_radix,
which is then converted to f64
@nrabulinski nrabulinski changed the title Int parsing Fix integer parsing Oct 2, 2021
@jedel1043
Copy link
Member

jedel1043 commented Oct 2, 2021

  • What does the spec say about even larger numbers? I've tested NodeJS and Firefox and you can more or less get unlimited range, albeit anything that's larger than Number.MAX_SAFE_INTEGER is not represented exactly. With this solution, however, numbers get clamped to -1.7e+38 and 1.7e+38. I'm assuming fixing this would most likely require an overhaul of the number representation and parsing code.

Now that I think about it, couldn't we use JsBigInt to parse the string as a big int, then convert into an f64? JsBigInt already has to_f64 and from_string_radix, so it should be an easy change.

@jedel1043 jedel1043 added this to the v0.14.0 milestone Oct 2, 2021
@jedel1043 jedel1043 added bug Something isn't working parser Issues surrounding the parser lexer Issues surrounding the lexer and removed parser Issues surrounding the parser labels Oct 2, 2021
@nrabulinski
Copy link
Contributor Author

Disregard my previous comment, I got confused 😄 but yes, we probably could use BigInt for that, should I look into this?

@jedel1043
Copy link
Member

Disregard my previous comment, I got confused 😄 but yes, we probably could use BigInt for that, should I look into this?

Please do!

@nrabulinski
Copy link
Contributor Author

Please do!

It seems to work correctly!

Copy link
Member

@jedel1043 jedel1043 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! :D

@raskad
Copy link
Member

raskad commented Oct 2, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 80,930 80,930 0
Passed 33,313 33,317 +4
Ignored 15,898 15,898 0
Failed 31,719 31,715 -4
Panics 0 0 0
Conformance 41.16% 41.17% +0.00%
Fixed tests (4):
test/built-ins/Number/prototype/toFixed/exactness.js [strict mode] (previously Failed)
test/built-ins/Number/prototype/toFixed/exactness.js (previously Failed)
test/built-ins/parseInt/S15.1.2.2_A7.2_T3.js [strict mode] (previously Failed)
test/built-ins/parseInt/S15.1.2.2_A7.2_T3.js (previously Failed)

@raskad raskad merged commit 5029a23 into boa-dev:master Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lexer Issues surrounding the lexer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants