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

Expand std::pin module docs and rename std::pin::Pinned to PhantomPinned #55992

Merged
merged 2 commits into from
Dec 12, 2018

Conversation

cramertj
Copy link
Member

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 15, 2018
@cramertj cramertj mentioned this pull request Nov 15, 2018
5 tasks
//!
//! Note that pinning and `Unpin` only affect the pointed-to type. For example, whether
//! or not `Box<T>` is `Unpin` has no affect on the behavior of `Pin<Box<T>>`. Similarly,
//! `Pin<Box<T>>` and `Pin<&mut T>` are always `Unpin` themselves, even though the
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be Box<T> and &mut T? We have an impl Unpin for Box<T> where T: !Unpin, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Those are also Unpin, but I was specifically referring to the Pin-wrapped types to clarify that Pin does not make something !Unpin.

src/libcore/pin.rs Outdated Show resolved Hide resolved
src/libcore/pin.rs Outdated Show resolved Hide resolved
//! changing the location of the underlying data, [`Pin`] prohibits accessing the
//! underlying pointer type (the `&mut` or `Box`) directly, and provides its own set of
//! APIs for accessing and using the value. [`Pin`] also guarantees that no other
//! functions will move the pointed-to value. This allows for the creation of
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we may want to say "no later code", or something along those lines, instead of "other functions". The contract around Pin talks about what cannot happen "in the future", but the current text doesn't currently say that.

@TimNN
Copy link
Contributor

TimNN commented Nov 27, 2018

Ping from triage @withoutboats: This PR requires your review.

@TimNN
Copy link
Contributor

TimNN commented Dec 4, 2018

Ping from triage @withoutboats / @rust-lang/libs: This PR requires your review.

@bors
Copy link
Contributor

bors commented Dec 8, 2018

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

@cramertj
Copy link
Member Author

Ping @rust-lang/libs can one of y'all make a decision here? I don't think there's much to be said that hasn't been already on the stabilization thread.

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

@rfcbot fcp merge

To confirm we're all on board with the renaming, but I suspect this won't take long

@rfcbot
Copy link

rfcbot commented Dec 10, 2018

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 a majority of reviewers approve (and none object), 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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 10, 2018
@rfcbot
Copy link

rfcbot commented Dec 12, 2018

🔔 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 Dec 12, 2018
@alexcrichton
Copy link
Member

Ok great! @cramertj want to rebase this and I'll r+?

@cramertj
Copy link
Member Author

@alexcrichton done!

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Dec 12, 2018

📌 Commit 709b751 has been approved by alexcrichton

@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 Dec 12, 2018
@bors
Copy link
Contributor

bors commented Dec 12, 2018

⌛ Testing commit 709b751 with merge 0076f58...

bors added a commit that referenced this pull request Dec 12, 2018
Expand std::pin module docs and rename std::pin::Pinned to PhantomPinned

cc #49150, #55766

r? @withoutboats
@bors
Copy link
Contributor

bors commented Dec 12, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 0076f58 to master...

@bors bors merged commit 709b751 into rust-lang:master Dec 12, 2018
@cramertj cramertj deleted the pin-docs branch December 13, 2018 00:31
@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 Dec 22, 2018
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. 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.

9 participants