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.

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

BTreeMap: split off most code of append #78417

Merged
merged 1 commit into from
Nov 12, 2020
Merged

BTreeMap: split off most code of append #78417

merged 1 commit into from
Nov 12, 2020

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

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

@bors r+

@bors
Copy link
Contributor

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
Contributor

bors commented Nov 9, 2020

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

@bors
Copy link
Contributor

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