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

BTreeMap: Support custom allocators #77438

Closed
wants to merge 4 commits into from

Conversation

exrook
Copy link
Contributor

@exrook exrook commented Oct 2, 2020

@rust-highfive
Copy link
Collaborator

r? @cramertj

(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 Oct 2, 2020
@bors
Copy link
Contributor

bors commented Oct 2, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@exrook exrook force-pushed the btreemap-alloc branch 2 times, most recently from 41c0200 to c7237fa Compare October 2, 2020 04:13
@exrook
Copy link
Contributor Author

exrook commented Oct 2, 2020

@rustbot modify labels: +A-allocators +T-libs

@rustbot rustbot added A-allocators Area: Custom and system allocators T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 2, 2020
@bors
Copy link
Contributor

bors commented Oct 3, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@TimDiekmann

This comment has been minimized.

@rustbot rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2020
library/alloc/src/collections/btree/map.rs Outdated Show resolved Hide resolved
library/alloc/src/collections/btree/map.rs Outdated Show resolved Hide resolved
@TimDiekmann
Copy link
Member

cc @Amanieu

@cramertj
Copy link
Member

cramertj commented Oct 5, 2020

r? @Amanieu

@bors
Copy link
Contributor

bors commented Oct 15, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Oct 16, 2020
BTreeMap: refactor Entry out of map.rs into its own file

btree/map.rs is approaching the 3000 line mark, splitting out the entry
code buys about 500 lines of headroom.

I've created this PR because the changes I've made in rust-lang#77438 will push `map.rs` over the 3000 line limit and cause tidy to complain.

I picked `Entry` to factor out because it feels less tightly coupled to the rest of `BTreeMap` than the various iterator implementations.

Related: rust-lang#60302
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 16, 2020
BTreeMap: refactor Entry out of map.rs into its own file

btree/map.rs is approaching the 3000 line mark, splitting out the entry
code buys about 500 lines of headroom.

I've created this PR because the changes I've made in rust-lang#77438 will push `map.rs` over the 3000 line limit and cause tidy to complain.

I picked `Entry` to factor out because it feels less tightly coupled to the rest of `BTreeMap` than the various iterator implementations.

Related: rust-lang#60302
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 17, 2020
BTreeMap: refactor Entry out of map.rs into its own file

btree/map.rs is approaching the 3000 line mark, splitting out the entry
code buys about 500 lines of headroom.

I've created this PR because the changes I've made in rust-lang#77438 will push `map.rs` over the 3000 line limit and cause tidy to complain.

I picked `Entry` to factor out because it feels less tightly coupled to the rest of `BTreeMap` than the various iterator implementations.

Related: rust-lang#60302
@bors
Copy link
Contributor

bors commented Oct 17, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@bors
Copy link
Contributor

bors commented Oct 21, 2020

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@rust-log-analyzer

This comment has been minimized.

@exrook exrook marked this pull request as ready for review March 23, 2022 12:00
@bors
Copy link
Contributor

bors commented Apr 6, 2022

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

@JohnCSimon
Copy link
Member

Triage:
@Amanieu is this ready for review?

Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

Sorry about the late review, this looks good modulo the leftover test code. Can you rebase to resolve the conflicts?

@@ -1684,6 +1684,7 @@ impl<'a> Builder<'a> {
rustflags.arg(&format!("-Cllvm-args=-import-instr-limit={}", limit));
}
}
cargo.env(profile_var("LTO"), "off");
Copy link
Member

Choose a reason for hiding this comment

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

Leftover from testing?

@Dylan-DPC Dylan-DPC 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 May 24, 2022
@TennyZhuang
Copy link
Contributor

What's the status of the PR? I'd like to help with something.

@Amanieu
Copy link
Member

Amanieu commented Jun 9, 2022

@TennyZhuang Since the author is not responding, you can take over by forking their code and opening a new PR.

@TennyZhuang
Copy link
Contributor

@exrook Sorry but I really need the feature, I'll take over the PR at the weekend. 🥰

@TennyZhuang
Copy link
Contributor

@Amanieu Hi, I've resolved the conflicts and submitted a new PR #98015, PTAL Thanks!

@Dylan-DPC
Copy link
Member

Closing this in favour of #98015

@Dylan-DPC Dylan-DPC closed this Jun 12, 2022
@exrook
Copy link
Contributor Author

exrook commented Jun 12, 2022

@TennyZhuang I'm glad to hear you're also looking to use this feature! My apologies for not responding sooner, I've been busy with other things, and when I last attempted a rebase on master back in May I was still hitting box-related ICEs so I've been working on a branch based on top of #95576, though it seems now those ICEs have been fixed in master :).

@Dylan-DPC Hi, could this be re-opened? I've put a lot of effort into keeping these changes in sync with master over the past year+ and a half and I'd like to see it through to being merged. :)

@Amanieu Thanks for the review! I've rebased on master and removed the leftover test code, so this should be good to go if you have no other comments

@TennyZhuang
Copy link
Contributor

@exrook I’m glad to hear that, and really hope the PR can be merged ASAP.
@Dylan-DPC @Amanieu Can you help to reopen the PR and review it again? Sorry for the extra review work.

@Amanieu
Copy link
Member

Amanieu commented Jun 13, 2022

GIthub doesn't let me reopen the PR after a force-push/rebase. You'll need to open a new one.

@TennyZhuang
Copy link
Contributor

TennyZhuang commented Jun 13, 2022

@exrook I’ve found that it’s possible to reopen the PR, but you have to revert the commits back to the one before closed, and commit them again, sorry.

See https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c

Feel free to reopen the PR or just create a new one.

@Dylan-DPC
Copy link
Member

@exrook unfortunately you can't reöpen this. You can submit a new pull request and link this one in the description and we can take it from there

@exrook
Copy link
Contributor Author

exrook commented Jun 14, 2022

Thanks everyone! I've opened a new PR at #98103

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jun 15, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 15, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-allocators Area: Custom and system allocators S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.