Add float conversions to and from bytes #62366

tesuji · 2019-07-04T09:50:35Z

Rework of #58756. Address #58756 (comment).

tesuji · 2019-07-04T10:04:06Z

info: removing rustup home
error: could not remove 'rustup_home' directory: '/usr/local/rustup'
info: caused by: Permission denied (os error 13)
Attempting to download https://.s3.amazonaws.com/docker/2f511ff28d2c3389996e6646724384337055dbe17514450f68a74b45672fa86ebd4e2373065cd78cea881bbf1f9ccb272982a872df86a3476451bf3eaec60ea7
Attempting with retry: curl -y 30 -Y 10 --connect-timeout 30 -f -L -C - -o /tmp/rustci_docker_cache https://.s3.amazonaws.com/docker/2f511ff28d2c3389996e6646724384337055dbe17514450f68a74b45672fa86ebd4e2373065cd78cea881bbf1f9ccb272982a872df86a3476451bf3eaec60ea7
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (6) Could not resolve host: .s3.amazonaws.com
Command failed. Attempt 2/5:
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (6) Could not resolve host: .s3.amazonaws.com
Command failed. Attempt 3/5:
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (6) Could not resolve host: .s3.amazonaws.com
Command failed. Attempt 4/5:
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (6) Could not resolve host: .s3.amazonaws.com
Command failed. Attempt 5/5:
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed

  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0curl: (6) Could not resolve host: .s3.amazonaws.com
The command has failed after 5 attempts.
open /tmp/rustci_docker_cache: no such file or directory

tesuji · 2019-07-04T10:10:34Z

Hm, the build failed. It seems I cannot mark those methods as const method.
I also need to_bits, from_bits to be const.

What should I do, @Centril?

Centril · 2019-07-04T11:12:43Z

What should I do, @Centril?

Not mark them as const =)

(or alternatively do what I suggested in #58756 (comment) but it is better not to...)

tesuji · 2019-07-04T13:13:49Z

r? @SimonSapin (It seems that scottmcm is busy)

tbu- · 2019-07-05T15:01:44Z

Thanks for picking this up!

SimonSapin · 2019-07-05T15:15:42Z

src/libcore/num/f32.rs

+    /// #![feature(float_to_from_bytes)]
+    /// let value = f32::from_be_bytes([0x40, 0x49, 0x0f, 0x67]);
+    /// let difference = (value - 3.141565f32).abs();
+    /// assert!(difference <= 1e-5);


Why not assert_eq!(value, 3.141565f32)?

I did the same check in from_bits method: https://github.com/rust-lang/rust/blob/e6c2844cad866f5495bffdb51d634a187c34ab76/src/libcore/num/f32.rs#L456-L463

Also I think that floating point equality comparison is incorrect and
discourage.

Also I think that floating point equality comparison is incorrect and
discourage.

Normally this is true, but if you just created the float using a bit pattern, it must be actually equal to the expected value.

I will let the decision to Simon.

Floating point equality can give unexpected results (compared to the same operation on real numbers) when it’s done on the result of non-trivial computation, because precision errors can acumulate. For example 0.3+0.3+0.3 != 0.9.

But as @tbu- said, the conversions here should be exact. Please change to from_*_bytes and from_bits doctests to use assert_eq!, and let’s see if the tests pass.

Also, please imitate from_bits / to_bits in picking a floating point value like 12.5 that has a “nice” representation, and using hex literals for integers.

I has a concern: What if people follow our style in their tests for
floating point equality?

Normally this is true, but if you just created the float using a bit pattern, it must be actually equal to the expected value.

Do we guarantee this for NaNs ? cc @rkruppe (As in, if one bitcasts a NaN bitpattern to a float, does LLVM guarantee that the bitpattern is preserved, or only that this float will be "some" NaN ?).

I highly doubt we have anything relevant explicitly written down. I don't know whether LLVM explicitly guarantees it for bitcasts only. I don't see why it wouldn't, but who knows. But even if LLVM explicitly stated that, that wouldn't mean we'd translate that into any Rust level guarantee.

FWIW, this PR does not introduce this problem. This is a problem that f32::from_bits and f64::from_bits already have, and these are stable, so I don't think clarifying this should hold this PR back. If we document this for those methods, that would just follow for these as well.

mark-i-m · 2019-07-06T19:27:28Z

cc @gnzlbg, who I think worked a bit on the equivalent for integers? May be good to make sure we have consistent APIs...

gnzlbg

The issue where this was raised had very little attention (cc @nagisa), so I would be more comfortable if the @rust-lang/libs team would at least comment on their overall feeling of adding these convenience methods, so that we don't add something here that then remains unstable forever (EDIT: seems that this concern was raised in #58756, and that the general feeling was that its ok to land these and stabilize them at some point).

I first wrote "I have never needed this", but actually I have! Well not precisely these, but their SIMD vector type equivalent (e.g. here (*)). AFAICT adding these isn't strictly necessary, since one can combine the existing methods to achieve the same effect, but I personally don't mind having them, and it is the kind of small utility that, if it isn't part of standard, it probably won't be worth pulling in an external crate for. So this change feels harmless to me, and if it helps some users, then that's great.

EDIT: These methods have the same API as the from/to_le/be/ne_bytes methods for integers, so that's consistent. We do guarantee that f32 and f64 are always 4 and 8 bytes wide, so this API should always be correctly implementable for all platforms that we support.

(*) that code came from std::simd, so it predates the from/to_..._bytes methods, and it has at best unspecified behavior today, because the union isn't repr(C), but union wasn't even stable back then.

SimonSapin · 2019-07-07T18:53:31Z

@lzutao You have a couple commits with “WIP” in the message. Do you mean to make further changes?

tesuji · 2019-07-08T01:21:43Z

No, I don't. Those WIP commits will be rebased after the review process completed.

SimonSapin · 2019-07-08T06:21:36Z

Ah ok. Here I was waiting for the changes to be completed, to finish reviewing :]

Anyway, the diff looks good now. r=me after you squash commits as desired.

Use the same API as for integers. Fixes rust-lang#57492.

SimonSapin · 2019-07-08T08:37:08Z

@bors r+

bors · 2019-07-08T08:37:10Z

📌 Commit df53a3f has been approved by SimonSapin

bors · 2019-07-08T08:40:06Z

⌛ Testing commit df53a3f with merge 10840b8...

@SimonSapin

Add float conversions to and from bytes Rework of #58756. Address #58756 (comment). Fixes #57492. r? @SimonSapin

bors · 2019-07-08T12:02:59Z

☀️ Test successful - checks-azure, checks-travis, status-appveyor
Approved by: SimonSapin
Pushing 10840b8 to master...

rust-highfive assigned scottmcm Jul 4, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 4, 2019

tesuji mentioned this pull request Jul 4, 2019

Add float conversions to and from bytes #58756

Closed

rust-highfive assigned SimonSapin and unassigned scottmcm Jul 4, 2019

SimonSapin mentioned this pull request Jul 5, 2019

Tracking issue for floats ↔ bytes conversions #60446

Closed

SimonSapin reviewed Jul 5, 2019

View reviewed changes

gnzlbg suggested changes Jul 7, 2019

View reviewed changes

tesuji force-pushed the feature/float-from-to-bytes branch from d3f69bb to b15e4aa Compare July 8, 2019 07:33

tbu- and others added 2 commits July 8, 2019 07:34

Add float conversions to and from bytes

ad47f08

Use the same API as for integers. Fixes rust-lang#57492.

Add documentation to float conversion methods

df53a3f

tesuji force-pushed the feature/float-from-to-bytes branch from b15e4aa to df53a3f Compare July 8, 2019 07:34

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 8, 2019

bors added a commit that referenced this pull request Jul 8, 2019

Auto merge of #62366 - lzutao:feature/float-from-to-bytes, r=SimonSapin

10840b8

Add float conversions to and from bytes Rework of #58756. Address #58756 (comment). Fixes #57492. r? @SimonSapin

bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 8, 2019

bors merged commit df53a3f into rust-lang:master Jul 8, 2019

tesuji deleted the feature/float-from-to-bytes branch July 8, 2019 12:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add float conversions to and from bytes #62366

Add float conversions to and from bytes #62366

tesuji commented Jul 4, 2019 •

edited

Loading

tesuji commented Jul 4, 2019

tesuji commented Jul 4, 2019

Centril commented Jul 4, 2019

tesuji commented Jul 4, 2019 •

edited

Loading

tbu- commented Jul 5, 2019

SimonSapin Jul 5, 2019 •

edited

Loading

tesuji Jul 5, 2019

tesuji Jul 5, 2019

tbu- Jul 5, 2019

tesuji Jul 5, 2019

SimonSapin Jul 5, 2019 •

edited

Loading

tesuji Jul 5, 2019

gnzlbg Jul 7, 2019 •

edited

Loading

hanna-kruppe Jul 7, 2019

gnzlbg Jul 7, 2019

mark-i-m commented Jul 6, 2019

gnzlbg left a comment •

edited

Loading

SimonSapin commented Jul 7, 2019

tesuji commented Jul 8, 2019

SimonSapin commented Jul 8, 2019

SimonSapin commented Jul 8, 2019

bors commented Jul 8, 2019

bors commented Jul 8, 2019

bors commented Jul 8, 2019

Add float conversions to and from bytes #62366

Add float conversions to and from bytes #62366

Conversation

tesuji commented Jul 4, 2019 • edited Loading

tesuji commented Jul 4, 2019

tesuji commented Jul 4, 2019

Centril commented Jul 4, 2019

tesuji commented Jul 4, 2019 • edited Loading

tbu- commented Jul 5, 2019

SimonSapin Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

tesuji Jul 5, 2019

Choose a reason for hiding this comment

tesuji Jul 5, 2019

Choose a reason for hiding this comment

tbu- Jul 5, 2019

Choose a reason for hiding this comment

tesuji Jul 5, 2019

Choose a reason for hiding this comment

SimonSapin Jul 5, 2019 • edited Loading

Choose a reason for hiding this comment

tesuji Jul 5, 2019

Choose a reason for hiding this comment

gnzlbg Jul 7, 2019 • edited Loading

Choose a reason for hiding this comment

hanna-kruppe Jul 7, 2019

Choose a reason for hiding this comment

gnzlbg Jul 7, 2019

Choose a reason for hiding this comment

mark-i-m commented Jul 6, 2019

gnzlbg left a comment • edited Loading

Choose a reason for hiding this comment

SimonSapin commented Jul 7, 2019

tesuji commented Jul 8, 2019

SimonSapin commented Jul 8, 2019

SimonSapin commented Jul 8, 2019

bors commented Jul 8, 2019

bors commented Jul 8, 2019

bors commented Jul 8, 2019

tesuji commented Jul 4, 2019 •

edited

Loading

tesuji commented Jul 4, 2019 •

edited

Loading

SimonSapin Jul 5, 2019 •

edited

Loading

SimonSapin Jul 5, 2019 •

edited

Loading

gnzlbg Jul 7, 2019 •

edited

Loading

gnzlbg left a comment •

edited

Loading