Implement conversion traits for primitive float types #29129

cuviper · Oct 17, 2015

This is a spiritual successor to #28921, completing the "upcast" idea from rust-num/num#97.

rust-highfive · Oct 17, 2015

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

petrochenkov · Oct 18, 2015

petrochenkov reviewed Oct 18, 2015

View changes

petrochenkov · Oct 18, 2015

src/libcoretest/num/mod.rs

@@ -177,4 +177,61 @@ mod tests {
    test_impl_from! { test_u16i32, u16, i32 }
    test_impl_from! { test_u16i64, u16, i64 }
    test_impl_from! { test_u32i64, u32, i64 }
+
+    // Signed -> Float
+    test_impl_from! { test_i8f32, i8, f32 }


I'd say conversions integer -> float feel... too suspicious for Into, even if they are lossless. I'd prefer to use more specialized methods/traits for this, at least for concrete types.
Do you have any example of generic code where this Into would be useful (I.e. a conversion from integer to float is required and it should be completely precise)?

I'll elaborate. I have a criterion for Into/AsRef: Suppose you have a function fn f(arg: Into<U>) (or try!, it also uses From for implicit conversions). Would you want it to implicitly accept Ts? If yes, then implementing Into<U> for T is reasonable. All current implementations meet this criterion (except for impl From<u32> for Ipv4Addr). Would you want fn f(arg: Into<f64>) to accept u8 implicitly? Probably not, there's a good chance this u8 is not your desired number, but its index, for example. I.e. type safety prevents mistakes. On the other hand, would you want it to accept u64 with explicit conversion? Probably yes, because zero error requirement is not normally important for such conversions.

Thanks for elaborating. I don't have a specific example already where this would be useful, but it's not hard to concoct one. Say log<F: Into<f64>>(self, base: F) -> f64, where you want the flexibility of float but the base is often integral. Yes, I think this could accept u8, but that's not really implicit since the function used Into, opting into that flexibility. The "type safety prevents mistakes" argument doesn't speak to me much, because you could say the same about even integral conversions, and again Into is an opt-in choice.

But I realize this is a pretty subjective thing to judge. Is there anyone else we should ping for an opinion about int->float? Do you at least agree with f32->f64?

Is there anyone else we should ping for an opinion about int->float?

@rust-lang/libs ?
(My opinion doesn't matter much, I've posted it because #28921 was mentioned)

Do you at least agree with f32->f64?

Yes!

@cuviper

Say log<F: Into<f64>>(self, base: F) -> f64, where you want the flexibility of float but the base is often integral.

In this example F == u64 is good too, because precise conversion is not required, so Into would probably be too strict.

My opinion doesn't matter much [...]

My opinion carries no more weight than yours. :) I appreciate your input!

alexcrichton · Oct 19, 2015

I think I've verified the integer parts, but is it true that all 32-bit floats can be represented losslessly as a 64-bit float? I think that's how floats work, but I just want to verify that's the case.

I agree with @petrochenkov that I'm a little hesitant here, but on the other hand I also don't feel too bad about having these From implementations. I'm totally fine with the f32 -> f64 conversion (pending my question above), and the others seem relatively natural to add.

cuviper · Oct 19, 2015

As long as we're talking IEEE 754, then for f32->f64 the exponent and significand are both larger, so I don't know any way there could be a lossy conversion. I hit almost all the weird cases I know in the new testcase. The only thing I didn't try was subnormal state -- I think the exponents work out such that all subnormal f32 values are still normal in f64. That's not really "lossy", but I can add a test for a subnormal round trip if you want.

alexcrichton · Oct 20, 2015

Ah nah it's fine to avoid subnormals and whatnot (I'm still not even 100% sure myself what those are), so this all sounds good to me! I've flagged this to come up during triage and we hopefully give some feedback on the "should we do this at all" aspect.

cuviper · Oct 20, 2015

I learned that subnormals (aka denormals) may also be zeroed depending on SSE flags, so testing those may be tricky or impossible anyway. (I don't know if Rust does anything with those flags.)

cuviper · Oct 27, 2015

cuviper reviewed Oct 27, 2015

View changes

cuviper · Oct 27, 2015

src/libcore/num/mod.rs

        impl From<$Small> for $Large {
-            #[stable(feature = "lossless_int_conv", since = "1.5.0")]
+            #[stable(feature = "lossless_prim_conv", since = "1.5.0")]


I guess if this isn't merged before 1.5 branches, I'll need a distinct macro and stable tag, right?

Although this will technically land during 1.6, I think these are fine here. We don't actually read these since versions anywhere, and it'd just be a pain to separate out this macro for 1.5 stable and 1.6 stable.

FYI, this PR is incorrectly listed in RELEASES.md under 1.5, possibly due to this confusion between the feature tag and the time of the actual merge. (I was wondering why my PR was listed but I wasn't in the contributor list...)

@brson -- need to tag this somehow so you can remember it for 1.6? It seems like "relnotes" needs to be version-specific.

huonw · Oct 28, 2015

huonw reviewed Oct 28, 2015

View changes

huonw · Oct 28, 2015

src/libcore/num/mod.rs

@@ -1514,3 +1514,20 @@ impl_from! { u8, i64 }
 impl_from! { u16, i32 }
 impl_from! { u16, i64 }
 impl_from! { u32, i64 }
+
+// Signed -> Float


Could you add a comment about the selection of types here, for future reference? (This is totally something I could imagine someone looking at and wondering about, especially since the details of floating point aren't in the front of everyone's head all the time.)

I.e. mention something about the precision (24 and 53 bits respectively) meaning these types being the only integers which can be represented losslessly in the float types.

Sure, I just pushed a new comment for this.

alexcrichton · Oct 29, 2015

The libs team discussed this during triage yesterday, and the decision was to move forward with this. Our thinking was along the lines that if you're generically taking Into<f32> then you're already willing to accept multiple types, so the "weirdness" of accepting an integer of an appropriate size is actually expected.

Thanks again for the PR @cuviper!

@bors: r+

bors · Oct 29, 2015

📌 Commit 1a19f98 has been approved by alexcrichton

bors · Oct 29, 2015

⌛️ Testing commit 1a19f98 with merge 4d11db6...

bors · Oct 29, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

Implement conversion traits for primitive float types

Loading status checks…

00d8d7b

rust-highfive assigned aturon Oct 17, 2015

alexcrichton added the T-libs label Oct 19, 2015

brson added the relnotes label Oct 28, 2015

Comment how the significand limits lossless int->float conversion

Loading status checks…

1a19f98

bors added a commit that referenced this pull request Oct 29, 2015

Auto merge of #29129 - cuviper:impl-from-for-floats, r=alexcrichton
This is a spiritual successor to #28921, completing the "upcast" idea from rust-num/num#97.

Loading status checks…

4d11db6

bors merged commit 1a19f98 into rust-lang:master Oct 29, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bors referenced this pull request Oct 29, 2015
Closed
Add additional automatic conversions to usize/isize. #29220

cuviper deleted the cuviper:impl-from-for-floats branch Mar 4, 2016

udoprog referenced this pull request Jan 30, 2018
Closed
Implement TryFrom for float to integer types #47857

rust-lang/rust

Join GitHub today

Implement conversion traits for primitive float types #29129

Conversation

cuviper commented Oct 17, 2015

rust-highfive assigned aturon Oct 17, 2015

This comment has been minimized.

rust-highfive commented Oct 17, 2015

petrochenkov reviewed Oct 18, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Oct 19, 2015

alexcrichton added the T-libs label Oct 19, 2015

This comment has been minimized.

cuviper commented Oct 19, 2015

This comment has been minimized.

alexcrichton commented Oct 20, 2015

This comment has been minimized.

cuviper commented Oct 20, 2015

cuviper reviewed Oct 27, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brson added the relnotes label Oct 28, 2015

huonw reviewed Oct 28, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton commented Oct 29, 2015

This comment has been minimized.

bors commented Oct 29, 2015

This comment has been minimized.

bors commented Oct 29, 2015

bors added a commit that referenced this pull request Oct 29, 2015

This comment has been minimized.

bors commented Oct 29, 2015

Hide details View details bors merged commit 1a19f98 into rust-lang:master Oct 29, 2015 2 checks passed

2 checks passed

bors referenced this pull request Oct 29, 2015

cuviper deleted the cuviper:impl-from-for-floats branch Mar 4, 2016

udoprog referenced this pull request Jan 30, 2018

petrochenkov reviewed Oct 18, 2015

View changes

cuviper reviewed Oct 27, 2015

View changes

huonw reviewed Oct 28, 2015

View changes

bors merged commit `1a19f98` into rust-lang:master Oct 29, 2015
2 checks passed