move the Pin API into its own module for centralized documentation #53227
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rust-highfive
added
the
S-waiting-on-review
|
Should |
|
I did as @withoutboats said in Resolution to outstanding questions:
I also assumed it meant keeping |
|
Yeah, I'm sort of two minds about this: |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Also thanks for taking the initiative on this @nivkner! |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I think I’d vote marker, but don’t feel super super strongly about it.
… On Aug 9, 2018, at 5:25 PM, nivkner ***@***.***> wrote:
Why are do the tests fail to build alloc, after successfully building it in stage 0?
I tested locally and everything passed.
And the failure doesn't make sense, Box is defined in alloc.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I do not have any opinion on where |
RalfJung
requested changes
Aug 13, 2018
|
|
||
| use boxed::Box; | ||
|
|
||
| /// A pinned, heap allocated reference. |
There was a problem hiding this comment.
This is a bit terse. It should explain briefly what the most important differences to Box are.
src/libcore/pin.rs
Outdated
| /// | ||
| /// A pinned reference is a lot like a mutable reference, except that it is not | ||
| /// safe to move a value out of a pinned reference unless the type of that | ||
| /// value implements the `Unpin` trait. |
There was a problem hiding this comment.
Might be worth mentioning some examples of what could be a move? mem::swap and mem::replace come to my mind.
src/libcore/pin.rs
Outdated
| /// may or may not implement `Unpin`. | ||
| /// | ||
| /// This constructor is unsafe because we do not know what will happen with | ||
| /// that data after the reference ends. If you cannot guarantee that the |
There was a problem hiding this comment.
"the lifetime of the reference"
src/liballoc/pin.rs
Outdated
| //! | ||
| //! see the [standard library module] for more information | ||
| //! | ||
| //! [standard library module]: ../../std/pin/index.html |
There was a problem hiding this comment.
Alternatively, this could re-export the stuff from core, so that the standard library module is just a full re-export of the one in liballoc. Isn't that what we usually do?
src/libstd/pin.rs
Outdated
| //! Types which pin data to its location in memory | ||
| //! | ||
| //! It is sometimes useful to have objects that are guaranteed to not move, | ||
| //! in the sense that their placement in memory in consistent, and can thus be relied upon. |
There was a problem hiding this comment.
"consistent" seems an odd choice of words to me, is that just because I am not a native speaker? It makes me wonder "consistent with what?".
What about "their placement in memory does not change, and"?
src/libstd/pin.rs
Outdated
| //! | ||
| //! In order to prevent objects from moving, they must be *pinned*, | ||
| //! by wrapping the data in special pointer types, such as [`PinMut`] and [`PinBox`]. | ||
| //! These restrict access to the underlying data to only be immutable by implementing [`Deref`], |
There was a problem hiding this comment.
The fact that getting &T works is an implementation detail.
The important point is that they restrict access to the underlying data such that it becomes impossible to move data out.
It then turns out that &T usually (without interior mutability) does not allow moving, but that is already the next step.
src/libstd/pin.rs
Outdated
| //! by wrapping the data in special pointer types, such as [`PinMut`] and [`PinBox`]. | ||
| //! These restrict access to the underlying data to only be immutable by implementing [`Deref`], | ||
| //! unless the type implements the [`Unpin`] trait, | ||
| //! which indicates that it doesn't need these restrictions and can be safely mutated, |
There was a problem hiding this comment.
Mutation is not the issue, moving is. We could add a method to PinMut which replaces its contents, it just cannot return the old content (but most drop it).
src/libstd/pin.rs
Outdated
| //! | ||
| //! This is done because, while modifying an object can be done in-place, | ||
| //! it might also relocate a buffer when its at full capacity, | ||
| //! or it might replace one object with another without logically "moving" them with [`swap`]. |
There was a problem hiding this comment.
I do not understand what you are trying to say here.
src/libstd/pin.rs
Outdated
| //! | ||
| //! // Now the only way to access to data (safely) is immutably, | ||
| //! // so this will fail to compile: | ||
| //! // still_unmoved.data.push_str(" world"); |
There was a problem hiding this comment.
Maybe change the example to use NonNull<str>... with NonNull<String>, just pushing more data to it does not change where the String itself lives.
|
One point that does not (yet) come up in these docs is the guarantee about |
|
IMO if we have a |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I don't understand why moving the pin module to liballoc causes a failure in a const evaluation test... |
|
Looks like just the line number changed? Not sure why that would happen though. Still, try running EDIT: Ah, yes. You changed |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
RalfJung
reviewed
Aug 21, 2018
src/liballoc/pin.rs
Outdated
| //! | ||
| //! In order to prevent objects from moving, they must be *pinned*, | ||
| //! by wrapping the data in special pointer types, such as [`PinMut`] and [`PinBox`]. | ||
| //! On top of ensuring the data cannot be taked by value by being pointers, |
There was a problem hiding this comment.
s/taked/taken/
But even than I do not understand what you are trying to say here.
RalfJung
reviewed
Aug 21, 2018
src/liballoc/pin.rs
Outdated
| //! by wrapping the data in special pointer types, such as [`PinMut`] and [`PinBox`]. | ||
| //! On top of ensuring the data cannot be taked by value by being pointers, | ||
| //! these types restrict access to the underlying data such that it cannot be moved out of them, | ||
| //! unless the type implements the [`Unpin`] trait, |
There was a problem hiding this comment.
Please end the sentence after "out of them" (it's getting pretty long) and start a new one for Unpin. Unpin deserves more than half a sentence. It should also mention that for Unpin types, we have PinMut<T> = &mut T and PinBox<T> = Box<T>, i.e. the pinned references "degenerate".
RalfJung
reviewed
Aug 21, 2018
src/liballoc/pin.rs
Outdated
| //! which indicates that it can be used safely without these restrictions. | ||
| //! | ||
| //! A type may be moved out of a reference to it using a function like [`swap`], | ||
| //! which replaces the contents of the references, and thus changes their place in memory. |
There was a problem hiding this comment.
This paragraph comes out of nowhere, it does not connect to the end of the previous paragraph. It should probably tie in with the introducing of PinMut/PinBox, and before Unpin.
I think after "such as [PinMut] and [PinBox]", I'd write something like "First of all, these are pointer types because pinned data cannot be passed around by value (that would change their location in memory). Secondly, since &mut and Box allow moving out the data from the pointer (e.g. through swap), we need dedicated pinned reference types that prohibit such operations."
This then transitions nicely to Unpin.
RalfJung
reviewed
Aug 21, 2018
src/libcore/pin.rs
Outdated
|
|
||
| //! Types which pin data to its location in memory | ||
| //! | ||
| //! see the [standard library module] for more information |
There was a problem hiding this comment.
Missing capitalization of see and final period.
|
This looks good. :-) The per-method documentation just got moved around without significant changes, right? |
|
Yeah, looks like it. Let's get this landed. Sorry that it took so long! @bors r+ |
|
📌 Commit 83ca347 has been approved by |
bors
added
S-waiting-on-bors
3 tasks
bors
added a commit
that referenced
this pull request
Aug 27, 2018
move the Pin API into its own module for centralized documentation This implements the change proposed by @withoutboats in #49150, as suggested by @RalfJung in the review of #53104, along with the documentation that was originally in it, that was deemed more appropriate in module-level documentation. r? @RalfJung
|
☀️ Test successful - status-appveyor, status-travis |
Merged
This was referenced Aug 29, 2018
nemosupremo
added a commit
to nemosupremo/tokio
that referenced
this pull request
Sep 3, 2018
carllerche
pushed a commit
to tokio-rs/tokio
that referenced
this pull request
Sep 4, 2018
The types moved in `std`. This patch updates tokio-async-await to import `PinMut` and `PinBox` from the new location. Ref: rust-lang/rust#53227
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This implements the change proposed by @withoutboats in #49150, as suggested by @RalfJung in the review of #53104,
along with the documentation that was originally in it, that was deemed more appropriate in module-level documentation.
r? @RalfJung