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

Implement placement-in protocol for Vec #38551

Merged
merged 1 commit into from
Jan 7, 2017

Conversation

aidanhs
Copy link
Member

@aidanhs aidanhs commented Dec 22, 2016

Follow-up of #32366 per comment at #30172 (comment), updating to latest rust, leaving @apasel422 as author and putting myself as committer.

I've removed the implementation of push in terms of place to make this PR more conservative.

@rust-highfive
Copy link
Collaborator

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@nagisa
Copy link
Member

nagisa commented Dec 23, 2016

The implementation looks fine to me. We might want to return a &mut T from the expression (i.e. set type of InPlace::Owner to &mut T), though; see rust-lang/rfcs#1821.

Otherwise its mostly for somebody from libs team to r+ it.

@aidanhs
Copy link
Member Author

aidanhs commented Dec 23, 2016

@nagisa ah thanks, I did wonder about that since returning a reference seems strictly more useful than not, but decided to follow in the footsteps of push and LinkedList::back_place. However, I've now updated the PR to return &mut T given the new information.

@aturon aturon added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Dec 23, 2016
@aturon
Copy link
Member

aturon commented Dec 23, 2016

cc @rust-lang/libs

@alexcrichton
Copy link
Member

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 25, 2016

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@brson
Copy link
Contributor

brson commented Jan 6, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Jan 6, 2017

📌 Commit 75fe66e has been approved by brson

@rfcbot
Copy link

rfcbot commented Jan 6, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

psst @alexcrichton, I wasn't able to add the final-comment-period label, please do so.

@bors
Copy link
Contributor

bors commented Jan 7, 2017

⌛ Testing commit 75fe66e with merge 3191886...

bors added a commit that referenced this pull request Jan 7, 2017
Implement placement-in protocol for `Vec`

Follow-up of #32366 per comment at #30172 (comment), updating to latest rust, leaving @apasel422 as author and putting myself as committer.

I've removed the implementation of `push` in terms of place to make this PR more conservative.
@bors
Copy link
Contributor

bors commented Jan 7, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: brson
Pushing 3191886 to master...

@bors bors merged commit 75fe66e into rust-lang:master Jan 7, 2017
@aidanhs aidanhs deleted the aphs-vec-in-place branch January 7, 2017 23:41
bors added a commit that referenced this pull request Jan 20, 2017
Implement placement-in protocol for `BinaryHeap`

Related to #30172, and loosley based on #38551.

At the moment, this PR is in a pretty rough state, but I wanted to get some feedback to see if I'm going in the right direction.

I hope the Mentor label of #30172 is still applicable, even though it's a year old 😄
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

10 participants