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.

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

Add associated constant BITS to all integer types #76492

Merged
merged 4 commits into from
Sep 19, 2020

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Sep 8, 2020

Recently I've regularly come across this snippet (in a few different crates, including core and std):

std::mem::size_of<usize>() * 8

I think it's time for a usize::BITS.

@m-ou-se m-ou-se changed the title Int bits Add associated constant BITS to all integer types Sep 8, 2020
@leonardo-m
Copy link

This is useful, good idea.
But call it NBITS? Or N_BITS? Or BIT_LEN? (In Python numbers have a bit_length() runtime method).

@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 8, 2020

I tried to follow the convention that was already used in this repository:

The name BITS is used in a few places for the number of bits in an integer (although only internally). N_BITS occurs zero times. The prefix N_ in general also occurs zero times (unless you count the rust-analyzer submodule, where it is used once).

@jyn514 jyn514 added 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. labels Sep 8, 2020
@camelid
Copy link
Member

camelid commented Sep 8, 2020

Bikeshed: BITSIZE? But it doesn't matter too much either way :)

@Mark-Simulacrum
Copy link
Member

Assigning to @dtolnay for T-libs hat "unstable API" review.

@leonardo-m
Copy link

The prefix N_ in general also occurs zero times

Nice, this shows how much different are coding habits ;-) And why I don't like to think as name choice as bikeshedding. Constant/function names of libraries must be chosen carefully if you want those libraries to be ergonomic :)

@bors
Copy link
Contributor

bors commented Sep 14, 2020

☔ The latest upstream changes (presumably #76678) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

This looks good. Please resolve the merge conflict in library/alloc/src/lib.rs and library/alloc/tests/string.rs and file a tracking issue, and r=me on the PR.

@dtolnay
Copy link
Member

dtolnay commented Sep 19, 2020

@bors delegate=m-ou-se

@bors
Copy link
Contributor

bors commented Sep 19, 2020

✌️ @m-ou-se can now approve this pull request

@dtolnay dtolnay added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2020
@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 19, 2020

@bors r=dtolnay rollup

@bors
Copy link
Contributor

bors commented Sep 19, 2020

📌 Commit 1bfe5ef has been approved by dtolnay

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 19, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 19, 2020
…r=dtolnay

Add associated constant `BITS` to all integer types

Recently I've regularly come across this snippet (in a few different crates, including `core` and `std`):
```rust
std::mem::size_of<usize>() * 8
```

I think it's time for a `usize::BITS`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 19, 2020
Rollup of 14 pull requests

Successful merges:

 - rust-lang#73963 (deny(unsafe_op_in_unsafe_fn) in libstd/path.rs)
 - rust-lang#75099 (lint/ty: move fns to avoid abstraction violation)
 - rust-lang#75502 (Use implicit (not explicit) rules for promotability by default in `const fn`)
 - rust-lang#75580 (Add test for checking duplicated branch or-patterns)
 - rust-lang#76310 (Add `[T; N]: TryFrom<Vec<T>>` (insta-stable))
 - rust-lang#76400 (Clean up vec benches bench_in_place style)
 - rust-lang#76434 (do not inline black_box when building for Miri)
 - rust-lang#76492 (Add associated constant `BITS` to all integer types)
 - rust-lang#76525 (Add as_str() to string::Drain.)
 - rust-lang#76636 (assert ScalarMaybeUninit size)
 - rust-lang#76749 (give *even better* suggestion when matching a const range)
 - rust-lang#76757 (don't convert types to the same type with try_into (clippy::useless_conversion))
 - rust-lang#76796 (Give a better error message when x.py uses the wrong stage for CI)
 - rust-lang#76798 (Build fixes for RISC-V 32-bit Linux support)

Failed merges:

r? `@ghost`
@bors bors merged commit fef3324 into rust-lang:master Sep 19, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 19, 2020
@leonardo-m
Copy link

size_of returns an usize, while BITS is a u32, so to replace one with the other I need to introduce a cast. Please consider making BITS a usize.

@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 21, 2020

Everything that counts bits of an integer type does so as a u32. E.g. leading_zeros, count_ones, the argument to rotate_left and wrapping_shr, etc.

@myrrlyn
Copy link

myrrlyn commented Sep 28, 2020

As another maintainer of a BITS constant, I'm very eager to see this occur.

Like leonardo-m, I also would prefer a usize rather than a u32, but given that we cannot change the existing u32 values to usize, I agree that consistency with what exists is more important than an inconsistent convenience. as usize is not overly burdensome to write.

@petrochenkov
Copy link
Contributor

Wait a second, what happened?
We had the BITS constants and they had the type usize - #23832.

@petrochenkov
Copy link
Contributor

Ah, here's what happened:

FIXME(#11621): Should be deprecated once CTFE is implemented in favour of calling the mem::size_of function.

Anyway, I also agree that they should be usize because they are almost always used in usize context.

@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 29, 2020

I don't agree usize is a good type for this. usize is meant for counting bytes in memory. It has exactly enough space to address all bytes in the address space. There's nothing like that for bits. If you want to count bits in all of the address space, you need something with three bits more, as there are eight times more bits than bytes. More importantly, this counts bits in the integer types, which is not related to the size of the address space. It does not need more space when switching from a 32 bit to a 64 bit system, unlike .len() etc.

All the existing functions that work with the number of bits in an integer (shifting, leading zeros, counting ones, etc.) work with u32. Patterns like u32::BITS - some_u32.leading_zeros() are very common.

I don't think the argument that it's mostly used as usize is true. This PR changed 24 places where mem::size_of * 8 was used to ::BITS. Only in seven cases is it casted to a usize. In quite a few cases it was combined with a function like leading_zeros() which also gives a u32. In a some cases, a as u32 was even removed.

Some examples from popular crates:

@leonardo-m
Copy link

BITS in bitvec is also not a usize, it's a u8

An advantage of it being u8 is that you can convert it to both u32 and usize without "as" and without unwraps.

yvt added a commit to r3-os/r3 that referenced this pull request Oct 1, 2020
`int_bits_const` feature [1] adds `i32::BITS`, etc. We already have
`BitInteger` trait, which has `BitInteger::BITS`, and using it triggers
`unstable_name_collision` compatibility lint [2] unless `int_bits_const`
is explicitly enabled, gets stabilized or gets rejected and removed from
the compiler.

[1]: rust-lang/rust#76492
[2]: rust-lang/rust#48919
yvt added a commit to r3-os/r3 that referenced this pull request Oct 1, 2020
`int_bits_const` feature [1] adds `i32::BITS`, etc. We already have
`BitInteger` trait, which has `BitInteger::BITS`, and using it triggers
`unstable_name_collision` compatibility lint [2] unless `int_bits_const`
is explicitly enabled, gets stabilized or gets rejected and removed from
the compiler.

[1]: rust-lang/rust#76492
[2]: rust-lang/rust#48919
yvt added a commit to r3-os/r3 that referenced this pull request Oct 1, 2020
`int_bits_const` feature [1] adds `i32::BITS`, etc. We already have
`BitInteger` trait, which has `BitInteger::BITS`, and using it triggers
`unstable_name_collision` compatibility lint [2] unless `int_bits_const`
is explicitly enabled, gets stabilized or gets rejected and removed from
the compiler.

[1]: rust-lang/rust#76492
[2]: rust-lang/rust#48919
@m-ou-se m-ou-se deleted the int-bits branch January 6, 2021 18:21
clint-white added a commit to clint-white/binary-heap-plus-rs that referenced this pull request Sep 23, 2022
This ports rust-lang/rust@1e2dba1e, part of rust-lang/rust#76492.
`usize::BITS` was stabilized in Rust 1.53.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

None yet

10 participants