rust-num / num-traits

Numeric traits for generic mathematics in Rust
Apache License 2.0
731 stars 135 forks source link

Replace `transmute()` for float casts with `to_bits()`? #123

Closed icefoxen closed 5 months ago

icefoxen commented 5 years ago

There's a few places where transmute() is used to turn a float into its bit representation. Rust 1.20 provides f32::to_bits() to do the same job, and it appears to be available in #[no_std]. Is there any particular reason not to use it over transmute(), since that would remove the only unsafe uses from this library?

cuviper commented 5 years ago

The only reason not to do this is compatibility, so I'm going to leave the issue open with that label. If we bump the minimum version in the future, we can certainly choose to call to_bits() instead.

In the meantime, I'm not too worried about the current use here, since the core implementation of that method is also just a transmute.

since that would remove the only unsafe uses from this library?

Generally speaking, while I appreciate the desire to reduce unsafe code, I don't want that to turn into total avoidance. Even if we remove the current instances of unsafe, we should feel free to add carefully-considered unsafe code in the future.

icefoxen commented 5 years ago

since the core implementation of that method is also just a transmute.

...Oops, I didn't realize that! Per the docs for f32::to_bits(): "Floats and Ints have the same endianness on all supported platforms." I feel a little silly, but oh well. Thanks!

nestordemeure commented 5 years ago

I was precisely about to post an issue because I could not find to_bits and needed it to convert a code from f64 to be generic over Float.

cuviper commented 5 years ago

This is about internally using f32/f64::to_bits() instead of transmute. Adding Float::to_bits() is a different question, complicated by the fact that the output needs an associated type. Please do file a separate issue if you want this -- I'd probably make it a new trait.

cuviper commented 5 months ago

We stopped using transmute for this in #240, commit fd601a8d4de1ab4e11c65929c679ae0c14fc7099.