Implement bit and set_bit for integral types.

[bjoernager]

View all comments

Tracking issue: #147702

This PR implements the bit and set_bit methods for integral types:

impl u8 {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

impl u16 {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

impl u32 {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

impl u64 {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

impl u128 {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

impl usize {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

impl i8 {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

impl i16 {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

impl i32 {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

impl i64 {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

impl i128 {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

impl isize {
    pub const fn bit(self, index: u32) -> bool;

    pub const fn set_bit(self, index: u32, value: bool) -> Self;
}

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[bjoernager]

@rustbot label +T-libs-api -T-compiler -T-libs

r? libs-api

[hanna-kruppe]

Not sure if this detail was discussed on the ACP, but the name set_bit reads to me like its signature should be (&mut self, i: u32, value: bool) -> () and not (as implemented here) (self, i: u32, value: bool) -> Self, which I'd call with_bit_set or with_bit.

[bjoernager]

Not sure if this detail was discussed on the ACP, but the name set_bit reads to me like its signature should be (&mut self, i: u32) -> () and not (as implemented here) (self, i: u32) -> Self, which I'd call with_bit_set.

Or perhaps just with_bit (given that value does not need to be set)? But I'm inclined to agree.

[hanna-kruppe]

I glossed over the value argument entirely when I wrote my first comment (edited now). Once I've noticed value I also have questions about it. To be honest, skimming the ACP discussion again, it seems like the setter half wasn't really clearly defined before being given a name and stamp of approval (e.g., two comments by @bjoernager disagreed over whether set_bit has a value parameter or not).

[bjoernager]

two comments by @bjoernager disagreed over whether set_bit has a value parameter or not).

My latter comment should've had the value parameter as well (fixed now).

[AljoschaMeyer]

Thank you @bjoernager for implementing (and probably championing) this 💜.

Fwiw I also think with_bit is probably the better name. If the doc comments of bit included a "see also with_bit" (and vice versa, for consistency), that could help people find the setter despite the perhaps initially unexpected name.

[bjoernager]

Suggestions seemed very rational to me and have all been implemented. Let me know if there are any other issues with the implementation, @dtolnay. :D

[folkertdev]

You might want to implement more test coverage in library/coretests/tests/num/int_macros.rs and /library/coretests/tests/num/uint_macros.rs. Don't forget to add the feature to library/coretests/tests/lib.rs.

These are also good suggestions that haven't been implemented yet (or not pushed?). The doc tests cover the logic but don't test e.g. const evaluation

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[bjoernager]

@rustbot ready

[bebecue]

The <$int>::bit() function is useful on its own and has received concerns so far. Shall we split this feature into two, like int_get_bit and int_set_bit? while stabilizing int_get_bit, int_set_bit could still receive further improvements. What do you think?

[clarfonthey]

r? libs

[clarfonthey]

I would expect this to just call self.cast_unsigned().bit(index) instead of rewriting the logic, to avoid duplicating the code.

View changes since the review

[clarfonthey]

Similarly would expect self.cast_unsigned().set_bit(index, value)

View changes since the review

[clarfonthey]

I get that operator precedence is on your side but parentheses help make this clearer:

            (self & ((1 as $SelfT) << index)) != (0 as $SelfT)

I'm also not entirely sure the as $SelfT is really necessary here:

Suggested change

	self & (1 as $SelfT) << index != (0 as $SelfT)
	(self & (1 << index)) != 0

View changes since the review

[clarfonthey]

Similarly, don't think you need the 1 as $SelfT:

Suggested change

	self &= !((1 as $SelfT) << index);
	self |= (value as $SelfT) << index;
	self
	self &= !(1 << index);
	self |= (value as $SelfT) << index;
	self

View changes since the review

[clarfonthey]

@rustbot author