Skip to content

Implement split_off for BTreeMap and BTreeSet (RFC 509) - #33947

Merged
bors merged 1 commit into
rust-lang:masterfrom
xosmig:btree_split_off
Jun 2, 2016
Merged

bors merged 1 commit into
rust-lang:masterfrom
xosmig:btree_split_off

Conversation

@xosmig

@xosmig xosmig commented May 29, 2016 •

Copy link
Copy Markdown
Contributor

Fixes #19986 and refactors common with append methods.
It splits the tree with O(log n) operations and then calculates sizes by traversing the lower one.

CC @gereeter

@rust-highfive

Copy link
Copy Markdown
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gankro (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Comment thread src/libcollections/btree/node.rs Outdated

// Source and destination must have the same height.
unsafe fn move_edges<K, V>(
source: Option<*mut BoxedNode<K, V>>, source_offset: usize,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think I'd prefer if this function took pointers not wrapped in Option - it makes more sense intuitively to me that you would check if you have edges to move, and if so, you call move_edges. I could be convinced otherwise though.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In fact, it might be even nicer to just take a NodeRef - it is easy to figure out where to place the edges.

@xosmig

xosmig commented May 29, 2016 •

Copy link
Copy Markdown
Contributor Author

Oh no. This is bad commit.

@xosmig

xosmig commented May 29, 2016

Copy link
Copy Markdown
Contributor Author

@gereeter, fixed

Comment thread src/libcollections/btree/map.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might be slightly nicer as a match on (split_edge.force(), right_node.force()).

@gereeter

Copy link
Copy Markdown
Contributor

Sans nits, this looks good to me.

Thank you, @xosmig!

@xosmig
xosmig force-pushed the btree_split_off branch from 21d582c to ca457e7 Compare May 30, 2016 08:28
@xosmig

xosmig commented May 30, 2016

Copy link
Copy Markdown
Contributor Author

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned Gankra May 30, 2016
@xosmig

xosmig commented May 30, 2016

Copy link
Copy Markdown
Contributor Author

@gereeter, Thanks for review!

@xosmig

xosmig commented May 30, 2016

Copy link
Copy Markdown
Contributor Author

should I squash it all into one commit?

@alexcrichton

Copy link
Copy Markdown
Member

@bors: delegate=gereeter

@bors

bors commented May 31, 2016

Copy link
Copy Markdown
Collaborator

✌️ @gereeter can now approve this pull request

@gereeter

gereeter commented Jun 1, 2016

Copy link
Copy Markdown
Contributor

Yes, I think that squashed PRs are standard practice, at least for collections changes like this that don't really have separate, logical steps.

@xosmig
xosmig force-pushed the btree_split_off branch from ca457e7 to e3adad6 Compare June 1, 2016 07:03
@gereeter

gereeter commented Jun 1, 2016

Copy link
Copy Markdown
Contributor

The Travis failure looks completely unrelated to this PR.

@bors r+

@bors

bors commented Jun 1, 2016

Copy link
Copy Markdown
Collaborator

📌 Commit e3adad6 has been approved by gereeter

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants