Implement conversion traits for primitive float types #29129

cuviper · 2015-10-17T23:31:23Z

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

rust-highfive · 2015-10-17T23:31:32Z

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 · 2015-10-18T01:44:55Z

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 · 2015-10-19T21:22:25Z

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 · 2015-10-19T21:56:41Z

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 · 2015-10-20T17:33:23Z

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 · 2015-10-20T18:44:33Z

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 · 2015-10-27T17:34:06Z

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 · 2015-10-28T22:31:46Z

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 · 2015-10-29T17:29:59Z

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 · 2015-10-29T17:30:00Z

📌 Commit 1a19f98 has been approved by alexcrichton

bors · 2015-10-29T18:55:13Z

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

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

bors · 2015-10-29T20:46:42Z

☀️ 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

00d8d7b

rust-highfive assigned aturon Oct 17, 2015

petrochenkov reviewed Oct 18, 2015
View reviewed changes

alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 19, 2015

cuviper reviewed Oct 27, 2015
View reviewed changes

brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 28, 2015

huonw reviewed Oct 28, 2015
View reviewed changes

Comment how the significand limits lossless int->float conversion

1a19f98

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

Auto merge of #29129 - cuviper:impl-from-for-floats, r=alexcrichton

4d11db6

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

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

bors mentioned this pull request Oct 29, 2015

Add additional automatic conversions to usize/isize. #29220

Closed

cuviper deleted the impl-from-for-floats branch March 4, 2016 08:54

udoprog mentioned this pull request Jan 30, 2018

Implement TryFrom for float to integer types #47857

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement conversion traits for primitive float types #29129

Implement conversion traits for primitive float types #29129

cuviper commented Oct 17, 2015

rust-highfive commented Oct 17, 2015

petrochenkov Oct 18, 2015

petrochenkov Oct 18, 2015

cuviper Oct 18, 2015

petrochenkov Oct 18, 2015

petrochenkov Oct 18, 2015

cuviper Oct 18, 2015

alexcrichton commented Oct 19, 2015

cuviper commented Oct 19, 2015

alexcrichton commented Oct 20, 2015

cuviper commented Oct 20, 2015

cuviper Oct 27, 2015

alexcrichton Oct 29, 2015

cuviper Dec 11, 2015

huonw Oct 28, 2015

cuviper Oct 28, 2015

alexcrichton commented Oct 29, 2015

bors commented Oct 29, 2015

bors commented Oct 29, 2015

bors commented Oct 29, 2015

Implement conversion traits for primitive float types #29129

Implement conversion traits for primitive float types #29129

Conversation

cuviper commented Oct 17, 2015

rust-highfive commented Oct 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Oct 19, 2015

cuviper commented Oct 19, 2015

alexcrichton commented Oct 20, 2015

cuviper commented Oct 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Oct 29, 2015

bors commented Oct 29, 2015

bors commented Oct 29, 2015

bors commented Oct 29, 2015