Expose float from_bits and to_bits in libcore. by clarfonthey · Pull Request #46931 · rust-lang/rust

clarfonthey · 2017-12-22T01:24:49Z

These methods have no dependencies on libm and thus should be offered in libcore.

rust-highfive · 2017-12-22T01:24:53Z

(rust_highfive has picked a reviewer for you, use r? to override)

est31 · 2017-12-22T06:54:29Z

Not sure why I didn't do it back in #39271. You should probably copy over the tests from std as well as the docs.

hanna-kruppe · 2017-12-22T12:40:07Z

It makes sense to have these operations in core, but note that we currently don't have a good way to expose those to end users. The Float trait is perma-unstable and doc(hidden), as it's mostly for internal purposes. Inherent methods can't work, as the inherent impl block is in std. Free functions in core::fN modules are unidiomatic.

This PR shouldn't/doesn't have to address that, I'm just saying this will not really be usable by anyone except other code in the standard library. There are indeed places in core that could use these methods (float parsing, and probably formatting too), so I'm in favor of this change.

est31 · 2017-12-22T19:28:34Z

@rkruppe thanks for explaining, probably that's why I didnt do it in my PR :). As this is for internal use only, the single line of docs that we have now is probably enough.

clarfonthey · 2017-12-22T22:06:48Z

Isn't the Float trait still in the core prelude? Similarly to the StrExt and SliceExt traits I just assumed that methods are available to use and the trait itself is not.

Long term I assume that the goal is to have multiple inherent impl blocks, so that core, alloc, and std could all have their own.

Regardless, I'm going to proceed with this and replace the existing unions that are used with to_bits and from_bits.

hanna-kruppe · 2017-12-22T22:11:04Z

Isn't the Float trait still in the core prelude? Similarly to the StrExt and SliceExt traits I just assumed that methods are available to use and the trait itself is not.

Nope: https://play.rust-lang.org/?gist=6b061936c1e1e0d872210e120f6d0ae3&version=stable

clarfonthey · 2017-12-23T02:55:59Z

@rkruppe I noticed! :(

Regardless, it appears that these methods would be useful in dec2flt, so, I converted that module to using the trait. It seems a bit messy with the associated const, but it works.

clarfonthey · 2017-12-23T03:03:58Z

 Zero | Subnormal | Normal => {
- let bits: u64 = x.transmute();
- T::from_bits(bits + 1)
+ T::from_bits(x.to_bits() + T::Bits::from(1u8))


This technically changes the overflow behaviour on release, but I don't think it should be a problem, as this function isn't expected to ever trigger the assert that was originally in from_bits here.

Yeah this can't overflow. If x was the all-ones bit pattern, it'd be a NaN and we wouldn't enter this branch.

hanna-kruppe · 2017-12-23T13:09:37Z

 pub trait Float: Sized {
+ /// Type used by `to_bits` and `from_bits`.
+ #[stable(feature = "core_float_bits", since = "1.24.0")]
+ type Bits: ops::Add<Output = Self::Bits> + From<u8> + TryFrom<u64>;


These bounds are pretty arbitrary. I guess it's okay for an internal trait, but alternatively you could move them to RawFloat (assuming that's the only place where they are needed).

I made this work by duplicating the associated type in the RawFloat trait.

bluss · 2017-12-31T14:30:11Z

To be clear -- they are not really offered as anything but doc(hidden) and unstable in libcore even with this PR, right? It seems we have no stable float methods with just libcore.

ollie27 · 2017-12-31T14:40:15Z

Marking the methods as stable in the Float trait will technically make them available in stable Rust. For example the following does compile on stable Rust:

#![no_std]#![crate_type="lib"]pubfnfoo() -> bool{::core::num::Float::is_nan(0f32)}

hanna-kruppe · 2017-12-31T14:51:16Z

Oh, right, that damn stability hole =/ Well, since this already applies to some existing methods, this PR doesn't really make it worse.

bluss · 2017-12-31T14:57:22Z

ugh. It's very unfortunate that in no_std, 0f32.is_nan() suggests use core::num::Float, but that use line is unstable.

bluss · 2017-12-31T14:58:42Z

I hope that we can reach the best kind of solution by providing inherent methods for f32, f64 in libcore (composable with the additional inherent methods in std)

shepmaster · 2018-01-06T02:41:42Z

Happy new year from triage, @bluss! Will you be able to check this out sometime?

shepmaster · 2018-01-13T00:48:13Z

Ping from triage, @bluss! It's been over 6 days since we've heard from you, maybe @rust-lang/libs could assign a new reviewer?

bluss · 2018-01-13T08:01:03Z

 assert!(significand < T::MIN_SIG, "encode_subnormal: not actually subnormal");
 // Encoded exponent is 0, the sign bit is 0, so we just have to reinterpret the bits.
- T::from_bits(significand)
+ T::from_bits(significand.try_into().unwrap_or_else(|_| unreachable!()))


This seems to be a step back, isn't there a way to keep this using just from_bits? Do you know if it has any performance impact?
The assertion assert!(bits < u32::MAX as u64, "f32::from_bits: too many bits"); from the old code has been lost, but I suppose it is unreachable here.

I wouldn't worry about performance impact, this is only called near the end of the slowest of slow paths. Edit: The other instance of this pattern, in encode_normal above, is also called in the slightly-less-slow-path, Algorithm R, but that path is still really slow in absolute terms. So I stand by my assesment.
It's still ugly as hell but that ugliness will have to be somewhere unless all the dec2flt code is restructured to work with the Bits associated type instead of u64 (which would probably not be pretty either). Maybe it could move into RawFloat::from_bits? (But if so, it should gain a nicer panic message than unreachable)

I'll try seeing what needs to be done to use Bits instead of u64 here.

That's probably far too big a refactor to be worthwhile for this PR.

Moving the try_into to from_bits seems more fruitful.

bluss · 2018-01-13T08:01:35Z

@shepmaster sure, that seems like a good idea

kennytm · 2018-01-17T11:48:50Z

Review ping for @rust-lang/libs! Randomly reassigning to another team member due to #46931 (comment).

r? @BurntSushi

Also @clarcharr could you address @bluss's comment in #46931 (review)?

BurntSushi · 2018-01-17T14:22:09Z

@rfcbot fcp merge

rfcbot · 2018-01-17T14:22:09Z

Team member @BurntSushi has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

carols10cents · 2018-01-22T22:16:38Z

Ping for ticky boxes here @Kimundi @dtolnay @sfackler!

rfcbot · 2018-01-23T21:04:54Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2018-01-23T21:04:54Z

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot · 2018-01-23T21:04:55Z

🔔 This is now entering its final comment period, as per the review above. 🔔

alexcrichton · 2018-01-23T22:11:34Z

@bors: r+

bors · 2018-01-23T22:11:35Z

📌 Commit 556fb02 has been approved by alexcrichton

bors · 2018-01-24T03:02:22Z

⌛ Testing commit 556fb02 with merge a538fe7...

Expose float from_bits and to_bits in libcore. These methods have no dependencies on libm and thus should be offered in libcore.

bors · 2018-01-24T06:19:25Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing a538fe7 to master...

dhardy · 2018-07-10T19:05:19Z

Did this really land in 1.25.0? Because I still get a compiler error with that version:

$ rustc -V
rustc 1.25.0 (84203cac6 2018-03-25)
$ cargo test --tests --no-default-features
Compiling serde v1.0.70
Compiling rand v0.5.3 (file:///home/dhardy/rust/rand)
error[E0599]: no function or associated item named `from_bits` found for type `f32` in the current scope
--> src/distributions/float.rs:98:22
|
98 | $ty::from_bits(self | exponent_bits)
| ^^^^^^^^^ function or associated item not found in `f32`
...
149 | float_impls! { f32, u32, f32, u32, 23, 127 }
| -------------------------------------------- in this macro invocation
|
= help: items from traits can only be used if the trait is in scope
= note: the following trait is implemented but not in scope, perhaps add a `use` for it:
candidate #1: `use core::num::Float;`
error[E0599]: no function or associated item named `from_bits` found for type `f64` in the current scope

Strangely I don't get warnings about conflicting keywords, which I got with older compilers (at least 1.22 and 1.23).

rust-highfive assigned bluss Dec 22, 2017

clarfonthey force-pushed the float_bits_core branch from f7fbd0f to 4a1a4a3Compare December 22, 2017 01:33

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 22, 2017

clarfonthey force-pushed the float_bits_core branch 3 times, most recently from 0cbe181 to 80d5bbfCompare December 23, 2017 01:18

Expose float from_bits and to_bits in libcore.
a2cdeb5

clarfonthey force-pushed the float_bits_core branch from 80d5bbf to a2cdeb5Compare December 23, 2017 02:53

clarfonthey commented Dec 23, 2017
View reviewed changes

hanna-kruppe reviewed Dec 23, 2017
View reviewed changes

Move Bits constraints to RawFloat::RawBits
556fb02

shepmaster added the T-libs-api [DEPRECATED; DO NOT USE] label Jan 13, 2018

bluss reviewed Jan 13, 2018
View reviewed changes

rust-highfive unassigned bluss Jan 17, 2018

rust-highfive assigned BurntSushi Jan 17, 2018

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 17, 2018

kennytm added S-waiting-on-team and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2018

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 23, 2018

bors added a commit that referenced this pull request Jan 24, 2018
Auto merge of#46931- clarcharr:float_bits_core, r=alexcrichton
a538fe7
Expose float from_bits and to_bits in libcore. These methods have no dependencies on libm and thus should be offered in libcore.

bors merged commit 556fb02 into rust-lang:masterJan 24, 2018

clarfonthey deleted the float_bits_core branch April 15, 2018 20:31

kjetilkjeka mentioned this pull request Apr 22, 2018
Unsafe encoding of floats can be removed now OpenCyphal-Garage/cyphal.rs#55
Closed

dhardy mentioned this pull request Jul 11, 2018
Fix build errors on nightly (alloc crate changes) rust-random/rand#545
Merged

Uh oh!

Uh oh!

Conversation

clarfonthey commented Dec 22, 2017

Uh oh!

rust-highfive commented Dec 22, 2017

Uh oh!

est31 commented Dec 22, 2017

Uh oh!

hanna-kruppe commented Dec 22, 2017

Uh oh!

est31 commented Dec 22, 2017

Uh oh!

clarfonthey commented Dec 22, 2017

Uh oh!

hanna-kruppe commented Dec 22, 2017

Uh oh!

clarfonthey commented Dec 23, 2017

Uh oh!

clarfontheyDec 23, 2017

Choose a reason for hiding this comment

Uh oh!

hanna-kruppeDec 23, 2017

Choose a reason for hiding this comment

Uh oh!

hanna-kruppeDec 23, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfontheyDec 23, 2017

Choose a reason for hiding this comment

Uh oh!

bluss commented Dec 31, 2017

Uh oh!

ollie27 commented Dec 31, 2017

Uh oh!

hanna-kruppe commented Dec 31, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bluss commented Dec 31, 2017

Uh oh!

bluss commented Dec 31, 2017

Uh oh!

shepmaster commented Jan 6, 2018

Uh oh!

shepmaster commented Jan 13, 2018

Uh oh!

blussJan 13, 2018

Choose a reason for hiding this comment

Uh oh!

hanna-kruppeJan 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clarfontheyJan 17, 2018

Choose a reason for hiding this comment

Uh oh!

hanna-kruppeJan 17, 2018

Choose a reason for hiding this comment

Uh oh!

hanna-kruppeJan 17, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bluss commented Jan 13, 2018

Uh oh!

kennytm commented Jan 17, 2018

Uh oh!

BurntSushi commented Jan 17, 2018

Uh oh!

rfcbot commented Jan 17, 2018• edited by Kimundi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

carols10cents commented Jan 22, 2018

Uh oh!

rfcbot commented Jan 23, 2018

Uh oh!

rfcbot commented Jan 23, 2018

Uh oh!

rfcbot commented Jan 23, 2018

hanna-kruppeDec 23, 2017•
edited
Loading

hanna-kruppe commented Dec 31, 2017•
edited
Loading

hanna-kruppeJan 17, 2018•
edited
Loading

hanna-kruppeJan 17, 2018•
edited
Loading

rfcbot commented Jan 17, 2018•
edited by Kimundi
Loading