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

Clamp to MAX / INFINITY rather than panic on overflow in number parsing #73

Merged
merged 5 commits into from Apr 2, 2015

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Apr 2, 2015

r? @jdm

This also changes the numeric types in tokens from f64 / i64 to f32 / i32, which might requires some changes in Servo.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented on src/tokenizer.rs in 6bfbfc1 Apr 2, 2015

This comment is confusing.

This comment has been minimized.

Copy link
Member Author

SimonSapin replied Apr 2, 2015

The change made it wrong, and I forgot to update it. Fixed in a follow-up commit.

@jdm

This comment has been minimized.

Copy link
Member

jdm commented on src/tokenizer.rs in 6bfbfc1 Apr 2, 2015

} else {

This comment has been minimized.

Copy link
Member Author

SimonSapin replied Apr 2, 2015

Done

@jdm
Copy link
Member

jdm commented Apr 2, 2015

I haven't looked at the merge commit; the rest look fine to me. Should I be looking at the merge?

@SimonSapin
Copy link
Member Author

SimonSapin commented Apr 2, 2015

The merge includes some fairly simple conflict resolutions, as well as a replacement of Float::powf with f64::powf (corresponding to a Rust upgrade).

By the way, the commit to use in Servo (unless Servo does another Rust upgrade) is the one before the merge.

@jdm
Copy link
Member

jdm commented Apr 2, 2015

I don't think there's a burning need to adopt these changes before the next upgrade. Looks good to me!

@SimonSapin
Copy link
Member Author

SimonSapin commented Apr 2, 2015

Looks good to me!

I’ll take that as r+ :)

SimonSapin added a commit that referenced this pull request Apr 2, 2015
Clamp to MAX / INFINITY rather than panic on overflow in number parsing
@SimonSapin SimonSapin merged commit 50aa60d into master Apr 2, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@SimonSapin SimonSapin deleted the overflow branch Apr 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.