Skip to content

index: add method for checking range on DenseBitSet #141871

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

Merged
merged 1 commit into from
Jun 4, 2025

Conversation

nia-e
Copy link
Contributor

@nia-e nia-e commented Jun 1, 2025

Micro-optimisation that Miri benefits from with the new isolated allocator for native-libs mode. Also possibly just a useful method to have on DenseBitSet

@rustbot
Copy link
Collaborator

rustbot commented Jun 1, 2025

r? @eholk

rustbot has assigned @eholk.
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 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. labels Jun 1, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jun 1, 2025

The Miri subtree was changed

cc @rust-lang/miri

@rust-log-analyzer

This comment has been minimized.

@nia-e nia-e force-pushed the fix-bitset branch 2 times, most recently from 001744c to 4164a82 Compare June 2, 2025 02:33
Copy link
Contributor

@eholk eholk left a comment

Choose a reason for hiding this comment

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

Could you also add a unit test for contains_any in compiler/rustc_index/src/bit_set/tests.rs?

let (start_word_index, start_mask) = word_index_and_mask(start);
let (end_word_index, end_mask) = word_index_and_mask(end);

if self.words[start_word_index] & (!start_mask | !(start_mask - 1)) != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like !start_mask | !(start_mask - 1) is always all ones. For example, if start_mask is 0b001000, then we have:

  • !(0b001000) | !(0b001000 - 1)
  • 0b110111 | !(0b000111)
  • 0b110111 | 0b111000
  • 0b111111

I think maybe you just want !(start_mask - 1), but let me know if I misunderstood.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok you're right, I tried to make it symmetric with the end_mask bit but I missed that. Thanks!

@rustbot rustbot 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 Jun 3, 2025
@nia-e
Copy link
Contributor Author

nia-e commented Jun 3, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 3, 2025
@rust-log-analyzer

This comment has been minimized.

@eholk
Copy link
Contributor

eholk commented Jun 3, 2025

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 3, 2025

📌 Commit 21f842d has been approved by eholk

It is now in the queue for this repository.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 3, 2025
@rust-log-analyzer

This comment has been minimized.

@nia-e
Copy link
Contributor Author

nia-e commented Jun 3, 2025

Gah, I think I know what's wrong. Should have let the tests run to the end locally, I think I fixed it and I'll push if so

@nia-e
Copy link
Contributor Author

nia-e commented Jun 3, 2025

Should be good? I'll wait for the CI before re-requesting approval just to be safe

@nia-e
Copy link
Contributor Author

nia-e commented Jun 3, 2025

@eholk you can r- this for now so it doesn't sit in the queue

@eholk
Copy link
Contributor

eholk commented Jun 3, 2025

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 3, 2025
@eholk
Copy link
Contributor

eholk commented Jun 3, 2025

@eholk you can r- this for now so it doesn't sit in the queue

Sure thing. Feel free to r=me once things are passing again, or ping me and I'll approve it again.

@nia-e
Copy link
Contributor Author

nia-e commented Jun 4, 2025

@bors r=eholk

@bors
Copy link
Collaborator

bors commented Jun 4, 2025

@nia-e: 🔑 Insufficient privileges: Not in reviewers

@nia-e
Copy link
Contributor Author

nia-e commented Jun 4, 2025

😔

@eholk

@eholk
Copy link
Contributor

eholk commented Jun 4, 2025

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jun 4, 2025

📌 Commit a0c19ee has been approved by eholk

It is now in the queue for this repository.

@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 Jun 4, 2025
bors added a commit that referenced this pull request Jun 4, 2025
Rollup of 8 pull requests

Successful merges:

 - #136687 (Improve the documentation of `Display` and `FromStr`, and their interactions)
 - #137306 (Remove `i128` and `u128` from `improper_ctypes_definitions`)
 - #138699 (build dist for x86_64-pc-solaris and sparcv9-sun-solaris)
 - #141250 (add s390x z17 target features)
 - #141467 (make `OsString::new` and `PathBuf::new` unstably const)
 - #141871 (index: add method for checking range on DenseBitSet)
 - #141888 (Use non-2015 edition paths in tests that do not test for their resolution)
 - #142000 (bootstrap: don't symlink source dir into stage0 sysroot)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e63e53a into rust-lang:master Jun 4, 2025
10 checks passed
@rustbot rustbot added this to the 1.89.0 milestone Jun 4, 2025
rust-timer added a commit that referenced this pull request Jun 4, 2025
Rollup merge of #141871 - nia-e:fix-bitset, r=eholk

index: add method for checking range on DenseBitSet

Micro-optimisation that Miri benefits from with the new isolated allocator for native-libs mode. Also possibly just a useful method to have on `DenseBitSet`
@nia-e nia-e deleted the fix-bitset branch June 4, 2025 14:57
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Jun 5, 2025
Rollup of 8 pull requests

Successful merges:

 - rust-lang/rust#136687 (Improve the documentation of `Display` and `FromStr`, and their interactions)
 - rust-lang/rust#137306 (Remove `i128` and `u128` from `improper_ctypes_definitions`)
 - rust-lang/rust#138699 (build dist for x86_64-pc-solaris and sparcv9-sun-solaris)
 - rust-lang/rust#141250 (add s390x z17 target features)
 - rust-lang/rust#141467 (make `OsString::new` and `PathBuf::new` unstably const)
 - rust-lang/rust#141871 (index: add method for checking range on DenseBitSet)
 - rust-lang/rust#141888 (Use non-2015 edition paths in tests that do not test for their resolution)
 - rust-lang/rust#142000 (bootstrap: don't symlink source dir into stage0 sysroot)

r? `@ghost`
`@rustbot` modify labels: rollup
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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants