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

Implement conversion traits for primitive float types #29129

Merged
merged 2 commits into from Oct 29, 2015

Conversation

cuviper
Copy link
Member

@cuviper cuviper commented Oct 17, 2015

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

@rust-highfive
Copy link
Collaborator

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.

@@ -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 }
Copy link
Contributor

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

@alexcrichton
Copy link
Member

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.

@alexcrichton 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
Copy link
Member Author

cuviper commented 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
Copy link
Member

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
Copy link
Member Author

cuviper commented 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.)

impl From<$Small> for $Large {
#[stable(feature = "lossless_int_conv", since = "1.5.0")]
#[stable(feature = "lossless_prim_conv", since = "1.5.0")]
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 28, 2015
@@ -1514,3 +1514,20 @@ impl_from! { u8, i64 }
impl_from! { u16, i32 }
impl_from! { u16, i64 }
impl_from! { u32, i64 }

// Signed -> Float
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I just pushed a new comment for this.

@alexcrichton
Copy link
Member

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
Copy link
Contributor

bors commented Oct 29, 2015

📌 Commit 1a19f98 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Oct 29, 2015

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

bors added a commit that referenced this pull request Oct 29, 2015
This is a spiritual successor to #28921, completing the "upcast" idea from rust-num/num#97.
@bors bors merged commit 1a19f98 into rust-lang:master Oct 29, 2015
@cuviper cuviper deleted the impl-from-for-floats branch March 4, 2016 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants