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

Add to_bytes and from_bytes to primitive integers #49871

Merged
merged 1 commit into from
Apr 14, 2018

Conversation

SimonSapin
Copy link
Contributor

Discussion issue turned tracking issue: #49792

@SimonSapin
Copy link
Contributor Author

r? @sfackler

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 11, 2018
@SimonSapin SimonSapin added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 11, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Apr 11, 2018

cc @BurntSushi

#[unstable(feature = "int_to_from_bytes", issue = "49792")]
#[inline]
pub fn to_bytes(self) -> [u8; mem::size_of::<Self>()] {
unsafe { mem::transmute(self) }
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need to be a transmute right? Can you do something like this instead?

unsafe { *(&self as *const Self as *const u8 as *[u8; mem::size_of::<Self>()]) }

Maybe we don't care about decreasing the use of transmute?

Also, neat, I didn't realize mem::size_of could be used at the type level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could be that, but that seems harder to read to me. Why would it be preferable? Regarding transmute I often prefer to fully qualify it with a turbofish, but in this case that felt unnecessary since it forms the entire body of a method that has an explicitly-written signature.

size_of became a const fn in #42859

Copy link
Contributor

Choose a reason for hiding this comment

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

I think transmute is fine here, because we're transmuting non-reference types. For reference/pointer types I agree that we should not be using transmute (this is what we suggest in clippy at least)

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oli-obk I’m not opposing this principle, but could you say more about why?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming the "why" refers to "transmute is fine for non-reference/pointer types":

let u = transmute::<T, U>(t); additionally checks whether the size of both sides of the conversion match. Going through raw pointer casts is essentially transmute_copy.

There's no increased source for errors when using transmute with all types specified (in this case specified in the function signature of the surrounding function). The raw pointer casts are very sigil heavy and need a few reads to figure out.

If you are transmuting references on the other hand, the size check doesn't gain you anything (because references to sized types are always one pointer large). In that case the raw pointer casts are also less sigil heavy because you don't need the all the dereference or reference expressions, but it's mostly a cast.

@BurntSushi
Copy link
Member

I love it! This should help remove some unsafe from byteorder.

/// Create an integer value from its memory representation as a byte array.
///
/// The target platform’s native endianness is used.
/// Portable code likely wants to use [`to_be`] or [`to_le`] after this.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: These should probably mention from_be and from_le instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it amusing that we have for example both to_be and from_be but their implementation is exactly the same. Anyway, fixed.

@sfackler
Copy link
Member

r=me with the one doc thing fixed

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2018
@SimonSapin
Copy link
Contributor Author

@bors: r=sfackler

@bors
Copy link
Contributor

bors commented Apr 14, 2018

📌 Commit 4472991 has been approved by sfackler

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 14, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Apr 14, 2018
Add to_bytes and from_bytes to primitive integers

Discussion issue turned tracking issue: rust-lang#49792
bors added a commit that referenced this pull request Apr 14, 2018
Rollup of 14 pull requests

Successful merges: #49908, #49876, #49916, #49951, #49465, #49922, #49866, #49915, #49886, #49913, #49852, #49958, #49871, #49864

Failed merges:
@bors bors merged commit 4472991 into rust-lang:master Apr 14, 2018
@SimonSapin SimonSapin deleted the int-bytes branch April 15, 2018 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

8 participants