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

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

r? libs-api

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.

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.

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).

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).

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.

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.

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.

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;

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'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.

@bjoernager ping

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.

@rustbot ready

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