Skip to content

Conversation

bjoernager
Copy link
Contributor

@bjoernager bjoernager commented Oct 14, 2025

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);
}

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 14, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 14, 2025

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
Copy link
Contributor Author

bjoernager commented Oct 14, 2025

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

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 14, 2025
@rustbot rustbot assigned dtolnay and unassigned Mark-Simulacrum Oct 14, 2025
@rustbot rustbot removed T-libs Relevant to the library team, which will review and decide on the PR/issue. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 14, 2025
@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Oct 14, 2025

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 bjoernager changed the title Implement bit and set_bit for integral types; Implement bit and set_bit for integral types. Oct 14, 2025
@bjoernager
Copy link
Contributor Author

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
Copy link
Contributor

hanna-kruppe commented Oct 14, 2025

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
Copy link
Contributor Author

bjoernager commented Oct 14, 2025

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

@rust-log-analyzer

This comment has been minimized.

@bjoernager bjoernager force-pushed the bit branch 2 times, most recently from 00e99a8 to de2e411 Compare October 14, 2025 19:41
@rust-log-analyzer

This comment has been minimized.

@AljoschaMeyer
Copy link
Contributor

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
Copy link
Member

Amanieu commented Oct 14, 2025

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
Copy link
Contributor

okaneco commented Oct 14, 2025

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

10 participants