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

is read_f32/read_f64 unsafe? #71

Closed
BurntSushi opened this issue Mar 1, 2017 · 2 comments
Closed

is read_f32/read_f64 unsafe? #71

BurntSushi opened this issue Mar 1, 2017 · 2 comments
Labels

Comments

@BurntSushi
Copy link
Owner

As of this moment, the read_f32 and read_f64 methods will bitcast any sequence of 4/8 bytes to a f32/f64 and will never fail, even if the resulting float is a "signaling NaN." In particular, materializing signaling NaNs appear to be undefined behavior, although the topic is pretty cloudy. See rust-lang/rust#39271 for more details.

I think what this means is that these functions need to be modified to return a Result and probably a custom error type as well, although it would be nice to just use the error type that we end up with from rust-lang/rust#39271.

This is a pretty major breaking change, so it will require a 2.0 release.

There is some confusion (at least in my head) around whether signaling NaN's are actually unsafe or not.

@rphmeier
Copy link

Is it actually a breaking change? I'm not sure a custom error type is needed, just that the returned value would be equal to the NAN constant if sNAN is detected.

@BurntSushi
Copy link
Owner Author

@rphmeier That's kind of missing the point though. :-) "Return an error" and "mask the sNaN" are two distinct but valid solutions to this particular problem. Technically, both are breaking changes. The former is obvious since it changes the type signature. The latter is subtle, but if users were previously depending on the specific bit values (say, for serialization purposes), then masking an sNaN will break that behavior. However, if sNaNs provoke undefined behavior (which legitimately seems unknown at this point), then I'm content qualifying it as a bug fix. :-)

See #70 for more details, where I think we decided to just mask the sNaN.

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

No branches or pull requests

2 participants