Implement bit and set_bit for integral types.

[bjoernager]

Tracking issue: #147702

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

impl /* u8 u16 u32 u64 u128 usize i8 i16 i32 i64 i128 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.

[Amanieu]

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.

with_set_bit is unfortunately quite a mouthful, and it can be mitigated by just adding #[must_use] to set_bit to indicate that it has no effects apart from its return value, which should avoid any accidental misuses.

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.

In the meeting we were specifically considering the version that takes a value: bool parameter.

[okaneco]

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.

[hanna-kruppe]

with_set_bit is unfortunately quite a mouthful, and it can be mitigated by just adding #[must_use] to set_bit to indicate that it has no effects apart from its return value, which should avoid any accidental misuses.

Agree on both points, but what about with_bit? That’s only one character more than set_bit and a better name than with_set_bit since, as @bjoernager pointed out, this function can also clear a bit.

Edit: the main reason I’m interested in this naming point is to leave room for the &mut self variant in case it’s added — I’m don’t care enough to seriously propose it now, but it does feel potentially useful. Non-mutating set_bit is awkward to use as replacement for some[long].place.expression |= 1 << idx;

[Amanieu]

I'm not a fan of with_bit because it doesn't clearly say that you are setting a bit to the given value.

[bjoernager]

I'm not a fan of with_bit because it doesn't clearly say that you are setting a bit to the given value.

I like to view it as bit denoting "a bit value" and with_bit denoting "with a bit value." Just like <*const _>::addr denotes "an address value" and <*const _>::with_addr denotes "with an address value."

In both cases, an original value (integer vs. pointer) has one of its subvalues (bit vs. address) overwritten completely.

[dtolnay]

Choosing a binary palindrome for the example misses the opportunity to orient the reader about the meaning of index.

Suggested change

	#[doc = concat!("let n = 0b1000001", stringify!($SelfT), ";")]
	///
	/// assert!(n.bit(0));
	/// assert!(!n.bit(1));
	#[doc = concat!("let n = 0b1000011", stringify!($SelfT), ";")]
	///
	/// assert!(n.bit(0));
	/// assert!(n.bit(1));

[dtolnay]

According to rust-lang/libs-team#667 (comment) the API that was accepted involves checking index in builds that contain overflow checking (debug mode or -C overflow-checks). I tested this and it did not panic:

#![feature(get_set_bit)]
fn main() {
    println!("{}", 0i32.bit(40));
}

whereas this does panic:

#![allow(arithmetic_overflow)]
fn main() {
    let index = 40;
    println!("{}", 0i32 & (1 as i32) << index != (0 as i32));
}

I think this attribute does the thing:

rust/library/core/src/num/int_macros.rs

Line 3289 in 5f9dd05

#[rustc_inherit_overflow_checks]