Skip to content

BTreeMap: split off most code of append#78417

Merged
bors merged 1 commit intorust-lang:masterfrom
ssomers:btree_chop_up_2
Nov 12, 2020
Merged

BTreeMap: split off most code of append#78417
bors merged 1 commit intorust-lang:masterfrom
ssomers:btree_chop_up_2

Conversation

@ssomers
Copy link
Contributor

@ssomers ssomers commented Oct 26, 2020 •
edited
Loading

To complete #78056, move the last single-purpose pieces of code out of map.rs into a separate module. Also, tweaked documentation and safeness - I doubt think this code would be safe if the iterators passed in wouldn't be as sorted as the method says they should be - and bounds on MergeIterInner.

r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2020
@jyn514 jyn514 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 29, 2020
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Nov 8, 2020

Hm, this seems fine, but I don't think we should make these methods unsafe without a justification for that -- if we have some unsafe code relying on sorted order in BTree, then that relies on PartialOrd/Ord returning consistent results, which is not necessarily true. Those can return a random choice of less/greater/equal to on each call, and we should not have any UB as a result of that (even if BTree is free to be quite confused and not return elements in the tree etc as a result). Ideally we wouldn't panic either but that's not too concerning, IMO, the important thing is that we don't have UB.

@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 Nov 8, 2020
@ssomers
Copy link
Contributor Author

ssomers commented Nov 8, 2020

Right. I did not realize the Ord bound wasn't even used. I'm not sure there is no UB to be wreaked from exploiting evil order, but I am pretty sure the methods involved here will not be the culprit.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Nov 8, 2020

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 8, 2020

📌 Commit 685fd53 has been approved by Mark-Simulacrum

@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 Nov 8, 2020
@bors
Copy link
Collaborator

bors commented Nov 9, 2020

⌛ Testing commit 685fd53 with merge 9264af3d945e005a0292a4a5ba0d23c08cb4d7a6...

@bors
Copy link
Collaborator

bors commented Nov 9, 2020

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 9, 2020
@m-ou-se
Copy link
Member

m-ou-se commented Nov 11, 2020

@bors retry

@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 Nov 11, 2020
This was referenced Nov 11, 2020
Closed
Closed
@jonas-schievink jonas-schievink mentioned this pull request Nov 11, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2020
@bors
Auto merge of rust-lang#78956 - jonas-schievink:rollup-r53giob, r=jon…
77180db 
…as-schievink

Rollup of 11 pull requests

Successful merges:

 - rust-lang#78216 (Duration::zero() -> Duration::ZERO)
 - rust-lang#78354 (Support enable/disable sanitizers/profiler per target)
 - rust-lang#78417 (BTreeMap: split off most code of append)
 - rust-lang#78832 (look at assoc ct, check the type of nodes)
 - rust-lang#78873 (Add flags customizing behaviour of MIR inlining)
 - rust-lang#78899 (Support inlining diverging function calls)
 - rust-lang#78923 (Cleanup and comment intra-doc link pass)
 - rust-lang#78929 (rustc_target: Move target env "gnu" from `linux_base` to `linux_gnu_base`)
 - rust-lang#78930 (rustc_taret: Remove `TargetOptions::is_like_android`)
 - rust-lang#78942 (Fix typo in comment)
 - rust-lang#78947 (Ship llvm-cov through llvm-tools)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 56e0806 into rust-lang:master Nov 12, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 12, 2020
@ssomers ssomers deleted the btree_chop_up_2 branch November 12, 2020 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.49.0

Development

Successfully merging this pull request may close these issues.

7 participants

@ssomers @Mark-Simulacrum @bors @m-ou-se @rust-highfive @jyn514 @rustbot