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.

Sign up for GitHub

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

Jump to bottom

Fix parsing floats panics and bugs #1110

Merged
merged 7 commits into from
Feb 4, 2021
Merged

Fix parsing floats panics and bugs #1110

merged 7 commits into from
Feb 4, 2021

Conversation

jevancc
Copy link
Contributor

@jevancc jevancc commented Feb 3, 2021

This Pull Request fixes/closes #1063 .

It changes the following:

  • Add dependency fast_float to parse float string
  • Fix parsing floats panic in number lexer
  • Fix bugs in parseFloat, parseInt, and Value::to_number

@codecov
Copy link

codecov bot commented Feb 3, 2021 •
edited
Loading

Codecov Report

Merging #1110 (c709594) into master (0fdfdfb) will increase coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1110      +/-   ##
==========================================
+ Coverage   58.81%   58.82%   +0.01%     
==========================================
  Files         176      176              
  Lines       12467    12484      +17     
==========================================
+ Hits         7332     7344      +12     
- Misses       5135     5140       +5
Impacted Files Coverage Δ
boa/src/value/mod.rs 72.70% <46.66%> (-0.99%) ⬇️
boa/src/builtins/string/mod.rs 62.91% <66.66%> (-0.08%) ⬇️
boa/src/builtins/number/mod.rs 66.31% <80.00%> (+1.07%) ⬆️
boa/src/syntax/lexer/number.rs 68.45% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0fdfdfb...36fd26e. Read the comment docs.

@RageKnify
Copy link
Member

Test262 conformance changes:

Test result master count PR count difference
Total 78,497 78,497 0
Passed 24,862 24,986 +124
Ignored 15,587 15,587 0
Failed 38,048 37,924 -124
Panics 18 16 -2
Conformance 31.67 31.83 +0.16%

Numbers look nice.

@RageKnify RageKnify added lexer Issues surrounding the lexer parser Issues surrounding the parser labels Feb 3, 2021
tofpie
tofpie approved these changes Feb 3, 2021
View reviewed changes
Copy link
Contributor

@tofpie tofpie 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! Thanks!

boa/src/builtins/string/mod.rs Outdated Show resolved Hide resolved
Lan2u
Lan2u approved these changes Feb 3, 2021
View reviewed changes
Copy link

@Lan2u Lan2u left a comment

Choose a reason for hiding this comment

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

Looks really good

@jevancc @tofpie
Update boa/src/builtins/string/mod.rs
36fd26e 
Co-authored-by: tofpie <75836434+tofpie@users.noreply.github.com>
@RageKnify RageKnify merged commit 9b1264f into boa-dev:master Feb 4, 2021
Razican pushed a commit that referenced this pull request May 22, 2021
@jevancc @tofpie @Razican
Fix parsing floats panics and bugs (#1110)
2f2daec 
* Fix parsing float panic

* Fix float parsing functions

* Fix parseFloat bugs

* Fix parseInt and parseFloat bugs

* Fix clippy

* Add todo comment

* Update boa/src/builtins/string/mod.rs

Co-authored-by: tofpie <75836434+tofpie@users.noreply.github.com>

Co-authored-by: tofpie <75836434+tofpie@users.noreply.github.com>
@RageKnify RageKnify added this to the v0.12.0 milestone May 31, 2021
@hle0 hle0 mentioned this pull request Oct 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tofpie tofpie tofpie approved these changes

@RageKnify RageKnify RageKnify approved these changes

@Lan2u Lan2u Lan2u approved these changes

@Razican Razican Awaiting requested review from Razican

Assignees
No one assigned
Labels
lexer Issues surrounding the lexer parser Issues surrounding the parser
Projects
None yet
Milestone
v0.12.0
Development

Successfully merging this pull request may close these issues.

Panic parsing some floats
4 participants
@jevancc @RageKnify @Lan2u @tofpie