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

Add examples for std::pin::Pin new() and into_inner() methods #104195

Closed
wants to merge 9 commits into from
Closed

Add examples for std::pin::Pin new() and into_inner() methods #104195

wants to merge 9 commits into from

Conversation

ch-iv
Copy link
Contributor

@ch-iv ch-iv commented Nov 9, 2022

Fixes #104107

@rustbot
Copy link
Collaborator

rustbot commented Nov 9, 2022

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 9, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 9, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

/// use std::pin::Pin;
///
/// let val: u8 = 5;
/// // Wrap the value in a pin to make sure it doesn't move
Copy link
Member

Choose a reason for hiding this comment

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

This isn't quite accurate. Since Target: Unpin, the value can move again.

Suggested change
/// // Wrap the value in a pin to make sure it doesn't move
/// // The value doesn't care about being moved, so we can pin it just fine

I don't like my wording here a lot, feel free to improve it, but something like this is better.

Copy link
Member

Choose a reason for hiding this comment

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

I guess that a better option is Pin the value in memory to make sure that during the program lifecycle the value is not moved in another memory address?

library/core/src/pin.rs Outdated Show resolved Hide resolved
Copy link
Member

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Some others comment regarding the PR are:

  • Use the Fixes https://github.com/rust-lang/rust/issues/104107 to make sure that the PR is linked with the issue
  • Try to squash the commit in a single one if the commit are simple the same things

Co-authored-by: Vincenzo Palazzo <[email protected]>
///
/// let val: u8 = 5;
/// // We can pin the value, since it doesn't care about being moved
/// let pinned: Pin<&u8> = Pin::new(&val);
Copy link
Member

Choose a reason for hiding this comment

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

It seems like an example of Pin with a shared reference is probably not the best bet -- most users will not want that. I think we should demonstrate with a &mut reference at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

    let mut val: u8 = 5;
    let mut pinned: Pin<&mut u8> = Pin::new(&mut val);
    println!("{}", pinned); // 5
    pinned.as_mut().set(10);
    println!("{}", pinned); // 10

Would this work?

@Mark-Simulacrum
Copy link
Member

@rustbot author

@rustbot rustbot 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 13, 2022
@yoshuawuyts
Copy link
Member

yoshuawuyts commented Nov 22, 2022

cc/ @rust-lang/wg-async I ran out of time to review this before going out of office. Can others take a look at this, perhaps during the next triage? I want to make sure we don't let this slip. Thanks!

@vincenzopalazzo
Copy link
Member

vincenzopalazzo commented Nov 22, 2022

@rustbot label +A-async-await

So in this way, it is indexed by penguin our friendly triage bot 😸

@rustbot rustbot added the A-async-await Area: Async & Await label Nov 22, 2022
@ch-iv
Copy link
Contributor Author

ch-iv commented Nov 23, 2022

@rustbot label -S-waiting-on-author +S-waiting-on-review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 23, 2022
@Mark-Simulacrum
Copy link
Member

r? @yoshuawuyts

@eholk
Copy link
Contributor

eholk commented Dec 6, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Dec 6, 2022

📌 Commit 7a47e2c has been approved by eholk

It is now in the queue for this repository.

@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 6, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 6, 2022
Add examples for std::pin::Pin new() and into_inner() methods

Fixes rust-lang#104107
@klensy
Copy link
Contributor

klensy commented Dec 6, 2022

https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy, commits should be rebased/squashed

@matthiaskrgr
Copy link
Member

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 6, 2022
@vincenzopalazzo
Copy link
Member

@ch-iv could you rebase and squash the commit in a single one? so we can get this in soon!

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 19, 2022
…=eholk

docs: improve pin docs

Override rust-lang#104195 with a full cleanup of the git history, now it should be ready to be merged.

r? `@eholk`

`@rustbot` label +A-async-await
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 19, 2022
…=eholk

docs: improve pin docs

Override rust-lang#104195 with a full cleanup of the git history, now it should be ready to be merged.

r? ``@eholk``

``@rustbot`` label +A-async-await
@ch-iv ch-iv closed this by deleting the head repository Jan 20, 2023
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request May 31, 2023
docs: improve pin docs

Override rust-lang/rust#104195 with a full cleanup of the git history, now it should be ready to be merged.

r? ``@eholk``

``@rustbot`` label +A-async-await
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve documentation for std::pin::Pin
10 participants