Skip to content

BTree: add drain methods #81075

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.

Sign up for GitHub

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

Jump to bottom
Closed
ssomers wants to merge 2 commits into from
Closed

BTree: add drain methods #81075

ssomers wants to merge 2 commits into from

Conversation

@ssomers
Copy link
Contributor

@ssomers ssomers commented Jan 16, 2021 •
edited
Loading

Implements #81074.
Builds on #93989.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 16, 2021
@ssomers ssomers mentioned this pull request Jan 16, 2021
Open
3 tasks
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jan 16, 2021

Is there a chance we can split this up into more commits? Reviewing a diff in a couple thousand lines is going to be quite challenging. It would be good to document any design decisions or otherwise guide my review, too.

@Mark-Simulacrum Mark-Simulacrum 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 Jan 16, 2021
@ssomers
Copy link
Contributor Author

ssomers commented Jan 16, 2021

I can split off a few refactorings of existing functions, to open up for new use, but they seemed a bit pointless on their own (apart from one thing that I actually postponed).

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Jan 16, 2021

I don't really know what you postponed, but I will likely not be able to effectively review this diff without at least some guidance as to where to start or the overall vision in adding these methods.

@ssomers
Copy link
Contributor Author

ssomers commented Jan 16, 2021 •
edited
Loading

The item I postponed is that steal_X functions (apart from their return value) are basically a hand-written optimization of the generic bulk_steal_X functions for count=1 (in reality, it went the other way around of course). It comes up here because one of the functions needed for drain is identical to handle_shrunk_node_recursively apart from that alleged optimization.

@ssomers
Copy link
Contributor Author

ssomers commented Jan 16, 2021

The overall vision in adding these methods is to not copy any existing functionality, but break it up in order to reuse parts. I'll feed those refactings in a few separate PRs now.

@ssomers ssomers mentioned this pull request Jan 16, 2021
Merged
@bors
Copy link
Collaborator

bors commented Jan 17, 2021

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

@ssomers ssomers mentioned this pull request Jan 18, 2021
Merged
@bors
Copy link
Collaborator

bors commented Jan 19, 2021

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

@ssomers ssomers mentioned this pull request Jan 25, 2021
Merged
@bors
Copy link
Collaborator

bors commented Jan 29, 2021

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

@bors
Copy link
Collaborator

bors commented Feb 9, 2021

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

@bors
Copy link
Collaborator

bors commented Feb 13, 2021

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

@bors
Copy link
Collaborator

bors commented Feb 15, 2021

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

@bors
Copy link
Collaborator

bors commented Feb 21, 2021

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 22, 2021
@bors
Auto merge of rust-lang#81362 - ssomers:btree_drainy_refactor_8, r=Ma…
a15f484 
…rk-Simulacrum

BTreeMap: gather and decompose reusable tree fixing functions

This is kind of pushing it as a standalone refactor, probably only useful for rust-lang#81075 (or similar).
r? `@Mark-Simulacrum`
@bors
Copy link
Collaborator

bors commented Feb 22, 2021

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

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2021
@bors
Auto merge of rust-lang#81094 - ssomers:btree_drainy_refactor_3, r=Ma…
3b150b7 
…rk-Simulacrum

BTreeMap: split up range_search into two stages

`range_search` expects the caller to pass the same root twice and starts searching a node for both bounds of a range. It's not very clear that in the early iterations, it searches twice in the same node. This PR splits that search up in an initial `find_leaf_edges_spanning_range` that postpones aliasing until the last second, and a second phase for continuing the search for the range in the each subtree independently (`find_lower_bound_edge` & `find_upper_bound_edge`), which greatly helps for use in rust-lang#81075. It also moves those functions over to the search module.

r? `@Mark-Simulacrum`
@ssomers
Copy link
Contributor Author

ssomers commented Mar 1, 2021

@rustbot label: -S-waiting-on-author +S-waiting-on-review

@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 Mar 1, 2021
@rustbot rustbot removed the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 16, 2021
@frank-king frank-king mentioned this pull request Jan 4, 2022
Closed
3 tasks
@ssomers ssomers changed the title BTree: add drain and split_off_range methods BTree: add remove_range and split_off_range methods Jan 4, 2022
@ssomers
Copy link
Contributor Author

ssomers commented Jan 4, 2022 •
edited
Loading

To stir up things, yanked the drain interface. I added remove_range instead just to have something that drops key-value pairs. Which only now makes me realize the drop_panic_leak test case for split_off_range is stupid: nothing is dropped before the split is finished.

PS also formatted code with stable instead of nightly and did not test tidy like a good boy should

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@ssomers ssomers changed the title BTree: add remove_range and split_off_range methods BTree: add split_off_range methods Jan 7, 2022
@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 Feb 6, 2022
@ssomers ssomers mentioned this pull request Feb 10, 2022
Closed
@ssomers ssomers changed the title BTree: add split_off_range methods BTree: add drain methods Feb 14, 2022
@ssomers ssomers mentioned this pull request Feb 14, 2022
Closed
@bors
Copy link
Collaborator

bors commented Feb 25, 2022

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

@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 Mar 20, 2022
@Dylan-DPC
Copy link
Member

Dylan-DPC commented Mar 26, 2022

r? @yaahc

@rust-highfive rust-highfive assigned yaahc and unassigned m-ou-se Mar 26, 2022
@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 Apr 24, 2022
@yaahc
Copy link
Member

yaahc commented Apr 29, 2022

r? rust-lang/libs-api

@rust-highfive rust-highfive assigned joshtriplett and unassigned yaahc Apr 29, 2022
@joshtriplett
Copy link
Member

joshtriplett commented May 1, 2022

r? rust-lang/libs-api

@rust-highfive rust-highfive assigned dtolnay and unassigned joshtriplett May 1, 2022
@bors
Copy link
Collaborator

bors commented Jun 16, 2022

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

@ssomers ssomers closed this Jun 18, 2022
@kekeimiku

This comment was marked as spam.

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

Reviewers

@the8472 the8472 the8472 left review comments

Assignees

@dtolnay dtolnay

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.