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

Stabilize Vec::leak as a method #74605

Merged
merged 2 commits into from
Aug 2, 2020
Merged

Stabilize Vec::leak as a method #74605

merged 2 commits into from
Aug 2, 2020

Conversation

SimonSapin
Copy link
Contributor

Closes #62195

The signature is changed to a method rather than an associated function:

-pub fn leak<'a>(vec: Vec<T>) -> &'a mut [T]
+pub fn leak<'a>(self) -> &'a mut [T]

The reason for Box::leak not to be a method (Deref to an arbitrary T which might have its own, different leak method) does not apply.

@SimonSapin SimonSapin added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jul 21, 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 Jul 21, 2020
@SimonSapin
Copy link
Contributor Author

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jul 21, 2020

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jul 21, 2020
@jonas-schievink jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label Jul 21, 2020
@jonas-schievink jonas-schievink added this to the 1.47 milestone Jul 21, 2020
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jul 22, 2020
@rfcbot
Copy link

rfcbot commented Jul 22, 2020

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 22, 2020
@bors
Copy link
Contributor

bors commented Jul 28, 2020

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

The reason for `Box::leak` not to be a method (`Deref` to an arbitrary `T`
which might have its own, different `leak` method) does not apply.
@KodrAus
Copy link
Contributor

KodrAus commented Jul 31, 2020

I’ll leave my comment here instead of the tracking issue because this is where the FCP is.

Is one of our main goals for leak methods to be “better” versions of into_raw? If that’s the case maybe we should consider making this return a tuple with the capacity as well:

let (slice, cap) = vec.leak();

let vec = unsafe { Vec::from_raw_parts(slice.as_mut_ptr(), slice.len(), cap) };

so that you have all the info you need to reconstitute the Vec with from_raw_parts?

@KodrAus
Copy link
Contributor

KodrAus commented Jul 31, 2020

If we want to leave leak as is though, maybe we could consider another leak_parts as well.

@Amanieu
Copy link
Member

Amanieu commented Jul 31, 2020

@KodrAus The point of leak is that it is safe and allows you have a 'static slice that is valid for the entire lifetime of the program. You lose this if you return a raw pointer instead.

@KodrAus
Copy link
Contributor

KodrAus commented Jul 31, 2020

@Amanieu Right. I think my question is easily answered by an orthogonal leak_parts and leaving this method as-is is better 👍

@SimonSapin
Copy link
Contributor Author

We already have into_raw_parts #65816 for cases where the goal is to later use from_raw_parts

@KodrAus
Copy link
Contributor

KodrAus commented Jul 31, 2020

Yes, we’ve been talking on that thread about *mut T vs NonNull<T>. I had some more thoughts on how we can keep from_raw/into_raw methods consistent across container types while still letting you work with a NonNull. I’ll write it up. I just wanted to see if it was orthogonal to this method first, and it is.

I do think the docs for Vec::leak should note that excess capacity will be dropped.

@SimonSapin
Copy link
Contributor Author

Not dropped in the Drop sense, but not be accessible anymore yes.

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 1, 2020
@rfcbot
Copy link

rfcbot commented Aug 1, 2020

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@Amanieu
Copy link
Member

Amanieu commented Aug 1, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 1, 2020

📌 Commit 7d759f5 has been approved by Amanieu

@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 Aug 1, 2020
@bors
Copy link
Contributor

bors commented Aug 1, 2020

⌛ Testing commit 7d759f5 with merge 5ef872f...

@bors
Copy link
Contributor

bors commented Aug 2, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: Amanieu
Pushing 5ef872f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 2, 2020
@bors bors merged commit 5ef872f into master Aug 2, 2020
@Mark-Simulacrum Mark-Simulacrum deleted the vec-leak branch August 19, 2020 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

Tracking issue for Vec::leak
9 participants