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.

Sign up for GitHub

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

Jump to bottom

Deprecate free-standing endian conversions in favor of methods on Int. Merge Bitwise into Int and add more bit operations. #14917

Closed
wants to merge 7 commits into from
Closed

Deprecate free-standing endian conversions in favor of methods on Int. Merge Bitwise into Int and add more bit operations. #14917

wants to merge 7 commits into from

Conversation

brendanzab
Copy link
Member

The free-standing endian conversion functions have been deprecated in favour of a group of conversion functions grouped together in the Int trait:

  • mem::from_be* -> Int::from_be
  • mem::from_le* -> Int::from_le
  • mem::to_be* -> Int::to_be
  • mem::to_le* -> Int::to_le

The Bitwise trait was also merged with Int. This further simplifies the trait hierarchy.

The get_bit, set_bit, clear_bit, and flip_bit methods were added to the Int trait.

[breaking-change]

core::num::Bitwise::swap_bytes was moved to core::num::Int

The free-standing endian conversion functions in core::mem have been deprecated and had their #[stable] attributes removed.

@alexcrichton
Copy link
Member

Note that this is explicitly moving some functions from #[stable] to #[deprecated], something we in theory never want to do.

I'm personally a little sad to see this expressed in yet another trait, and I'm not a huge fan of to_little_endian over to_le, but that's mostly a matter of taste.

@kud1ing
Copy link

kud1ing commented Jun 16, 2014

le could also mean less or equal.

@brendanzab
Copy link
Member Author

Ok, I removed the deprecations.

@brendanzab brendanzab changed the title Group endian conversion functions in a single trait Add a ByteOrder trait for abstracting over endian conversions Jun 16, 2014
@huonw
Copy link
Member

huonw commented Jun 16, 2014

Do we actually want to be "stuck" with the old #[stable] functions?

@alexcrichton
Copy link
Member

I'm not necessarily saying that we're stuck with #[stable] functions forever. I just wanted to point out that we're changing #[stable] functions.

@brendanzab
Add a ByteOrder trait for abstracting over endian conversions
ef9c152 
The `Bitwise::swap_bytes` method was also moved into the `ByteOrder` trait. This was because it works on the byte level rather than the bit level.
@brendanzab
Use ByteOrder methods instead of free-standing functions
a2045b2
@brendanzab
Fix comment formatting
57cfe5b
@brendanzab
Copy link
Member Author

Ok, me and @alexcrichton discussed this, and decided it was probably better just to merge the bit and bytewise stuff into the Int trait. The resulting API looks much cleaner.

alexcrichton
alexcrichton reviewed Jun 16, 2014
View reviewed changes
src/libcore/num/mod.rs
///
/// # Example
///
/// ```rust
/// use std::num::Bitwise;
/// static N: u8 = 0b01001100;
Copy link
Member

Choose a reason for hiding this comment

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

Why change these to statics?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, seemed more clear. I can change them back though.

@brendanzab brendanzab changed the title Add a ByteOrder trait for abstracting over endian conversions Deprecate free-standing endian conversions in favor of methods on Int. Merge Bitwise into Int. Jun 16, 2014
@alexcrichton
Copy link
Member

In general, this seems good to me. I still prefer to_le over to_little_endian, but that's probably just me. I think this needs a few more eyes, however.

@brendanzab
Copy link
Member Author

Sure thing.

@brendanzab
Copy link
Member Author

@alexcrichton de-staticised

@emberian
Copy link
Member

cc me

@emberian
Copy link
Member

LGTM. I also prefer the brevity of to_le, but the expanded form is much more clear.

@brendanzab brendanzab changed the title Deprecate free-standing endian conversions in favor of methods on Int. Merge Bitwise into Int. Deprecate free-standing endian conversions in favor of methods on Int. Merge Bitwise into Int anmd add more bit operations. Jun 17, 2014
@brendanzab brendanzab changed the title Deprecate free-standing endian conversions in favor of methods on Int. Merge Bitwise into Int anmd add more bit operations. Deprecate free-standing endian conversions in favor of methods on Int. Merge Bitwise into Int and add more bit operations. Jun 17, 2014
@brendanzab
Copy link
Member Author

Speaking to @aturon over lunch, he also prefers the shorter names. I will alter the patch to use the shorter names them.

@brendanzab
Copy link
Member Author

Any more feedback on this?

@brendanzab
Shorten endian conversion method names
59cd6f0 
The consensus on rust-lang#14917 was that the proposed names were too long.
@alexcrichton
Copy link
Member

We chatted on IRC. r=me if you remove the new {set,get,flip,clear}_bit functions.

alexcrichton pushed a commit to alexcrichton/rust that referenced this pull request Jun 19, 2014
@brendanzab @alexcrichton
Shorten endian conversion method names
cb8ca2d 
The consensus on rust-lang#14917 was that the proposed names were too long.
@FreeFull FreeFull mentioned this pull request Jun 20, 2014
Merged
@brendanzab brendanzab deleted the byte-order branch October 24, 2014 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brendanzab @alexcrichton @kud1ing @huonw @emberian