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

Integrate binary search codes of binary_search_by and partition_point #85406

Merged
merged 1 commit into from
Jun 16, 2021

Conversation

VillSnow
Copy link
Contributor

For now partition_point has own binary search code piece.
It is because binary_search_by had called the comparer more times and the author (=me) wanted to avoid it.

However, now binary_search_by uses the comparer minimum times. (#74024)
So it's time to integrate them.

The appearance of the codes are a bit different but both use completely same logic.

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 17, 2021
@Folyd
Copy link
Contributor

Folyd commented May 28, 2021

I'd recommend updating the docs of slice::binary_search* to clarify the necessity of partition_point() if they need determinism results. Since the Polkadot had an accident related to my PR recently. See: #85773
What do you think? @VillSnow @m-ou-se

@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 14, 2021
@JohnTitor
Copy link
Member

I'd recommend updating the docs of slice::binary_search* to clarify the necessity of partition_point() if they need determinism results. Since the Polkadot had an accident related to my PR recently.

Sounds good! But I think it's unrelated to this PR itself and can be done in another PR.

@JohnTitor
Copy link
Member

r? @JohnTitor @bors r+

@bors
Copy link
Contributor

bors commented Jun 14, 2021

📌 Commit 5db13c5 has been approved by JohnTitor

@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 14, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Jun 14, 2021
…=JohnTitor

Integrate binary search codes of binary_search_by and partition_point

For now partition_point has own binary search code piece.
It is because binary_search_by had called the comparer more times and the author (=me) wanted to avoid it.

However, now binary_search_by uses the comparer minimum times. (rust-lang#74024)
So it's time to integrate them.

The appearance of the codes are a bit different but both use completely same logic.
@bors
Copy link
Contributor

bors commented Jun 15, 2021

⌛ Testing commit 5db13c5 with merge 684ca33...

@bors
Copy link
Contributor

bors commented Jun 16, 2021

☀️ Test successful - checks-actions
Approved by: JohnTitor
Pushing 684ca33 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 16, 2021
@bors bors merged commit 684ca33 into rust-lang:master Jun 16, 2021
@rustbot rustbot added this to the 1.55.0 milestone Jun 16, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 17, 2021
…ochenkov

Prefer `partition_point` to look up assoc items

Since we now have `partition_point` (instead of `equal_range`), I think it's worth trying to use it instead of manually finding it.
`partition_point` uses `binary_search_by` internally (rust-lang#85406) and its performance has been improved (rust-lang#74024), so I guess this will make a performance difference.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants