Skip to content
This repository has been archived by the owner on Mar 25, 2024. It is now read-only.

Improved serialization/deserialization for floating point numbers. #88

Merged
merged 1 commit into from
Jan 21, 2018

Conversation

frewsxcv
Copy link
Contributor

@frewsxcv frewsxcv commented Jan 20, 2018

"50." --(deserialize)--> yaml::Number::from(50.0) --(serialize)--> "50" --(deserialize)--> yaml::Number::from(50)

  • deserialize/serialize inf/-inf/nan
  • always output decimal point for truncated floating points

@killercup
Copy link
Contributor

I assume you found this with rust-fuzz/targets#99? :)

src/ser.rs Outdated
@@ -73,7 +73,11 @@ impl ser::Serializer for Serializer {
}

fn serialize_f64(self, v: f64) -> Result<Yaml> {
Ok(Yaml::Real(v.to_string()))
Ok(Yaml::Real(if v.trunc() == v {
Copy link
Contributor

Choose a reason for hiding this comment

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

error: strict comparison of f32 or f64
  --> src/ser.rs:76:26
   |
76 |         Ok(Yaml::Real(if v.trunc() == v {
   |                          ^^^^^^^^^^^^^^ help: consider comparing them within some error: `(v.trunc() - v).abs() < error`

Copy link
Owner

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

How about switching to dtoa instead? That way we avoid printing big exponents like 25e200f64 as a bunch of garbage digits.

2499999999999999754368647829910432568018032091847598320736246101108455787066727664225769304644938620168687085525975646159967957760326637205079237977314680335729196571361796923253538665328348131219210240.

@frewsxcv
Copy link
Contributor Author

@dtolnay one issue with dtoa is that it doesn't handle std::f32::INFINITY correctly. dtoa outputs 1.797693134862316e308 and the current implementation outputs inf., which is also wrong. thoughts?

@dtolnay
Copy link
Owner

dtolnay commented Jan 20, 2018

From 10.2.1.4. Floating Point it looks like the correct values would be .inf, -.inf, .nan. We would need to handle those cases before calling into dtoa.

- deserialize/serialize inf/-inf/nan
- always output decimal point for truncated floating points
@frewsxcv frewsxcv changed the title Always include decimal point when serializing floating point numbers. Improved serialization/deserialization for floating point numbers. Jan 21, 2018
@frewsxcv
Copy link
Contributor Author

@dtolnay how's this look?

let float: f64 = serde_yaml::from_str(&unindent("
---
.nan")).unwrap();
assert!(float.is_nan());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can't compare a nan with a nan, so gotta do the is_nan dance 💃

@dtolnay
Copy link
Owner

dtolnay commented Jan 21, 2018

Looks good! I filed #89 to follow up on some odd cases. I don't think trim_left_matches('+') is what we want but there is other sketchy code related to + above it anyway.

@dtolnay dtolnay merged commit 18725af into dtolnay:master Jan 21, 2018
@frewsxcv frewsxcv deleted the frewsxcv-f64-ser branch January 21, 2018 03:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants