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

Document behavior of ptr::swap with overlapping regions of memory. #46483

Merged
merged 1 commit into from
Dec 6, 2017

Conversation

frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Dec 3, 2017

Fixes #44479.

@rust-highfive
Copy link
Collaborator

r? @BurntSushi

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

/// otherwise equivalent.
/// deinitializing either. The values pointed at by `x` and `y` may overlap,
/// unlike `mem::swap` which is otherwise equivalent. If the values do overlap,
/// then the overlapping region of memory will be derived from `x`.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I understood the last sentence here until I looked at the example. Is there any way to write this more clearly? (The answer might be "no." I don't have any particularly good ideas.)

Copy link
Member Author

Choose a reason for hiding this comment

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

another wording that comes to mind:

If the values do overlap, then the overlapping region of memory from x takes priority.

Copy link
Member

Choose a reason for hiding this comment

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

That does actually seem better to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

adjusted in the latest force push

Copy link
Contributor

Choose a reason for hiding this comment

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

I have to be honest, I didn't understand "takes priority" and I find the original wording better :/

Copy link
Contributor

Choose a reason for hiding this comment

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

"will be used" is another possibility. It might also make sense to simply be like "see the example below" since this is definitely something an example clarifies greatly.

Copy link
Member Author

Choose a reason for hiding this comment

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

applied @gankro's suggestions in the latest force push

--- a/src/libcore/ptr.rs
+++ b/src/libcore/ptr.rs
@@ -91,9 +91,12 @@ pub const fn null<T>() -> *const T { 0 as *const T }
 pub const fn null_mut<T>() -> *mut T { 0 as *mut T }
 
 /// Swaps the values at two mutable locations of the same type, without
-/// deinitializing either. The values pointed at by `x` and `y` may overlap,
-/// unlike `mem::swap` which is otherwise equivalent. If the values do overlap,
-/// then the overlapping region of memory from `x` takes priority.
+/// deinitializing either.
+///
+/// The values pointed at by `x` and `y` may overlap, unlike `mem::swap` which
+/// is otherwise equivalent. If the values do overlap, then the overlapping
+/// region of memory from `x` will be used. This is demonstrated in the
+/// examples section below.
 ///
 /// # Safety
 ///

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

This looks good to me and appears consistent with what we agreed on in #44479. I did leave one comment about clarifying the docs, but you might say that the current PR is good enough!

cc @gankro @rust-lang/libs

@Gankra
Copy link
Contributor

Gankra commented Dec 4, 2017

sgtm

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 4, 2017
@steveklabnik
Copy link
Member

I'm also a little unsure about how to phrase this, but if we can't think of anything, we should :shipit:

@kennytm kennytm added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Dec 4, 2017
@frewsxcv
Copy link
Member Author

frewsxcv commented Dec 6, 2017

anyone else have thoughts here? someone wanna approve this?

@BurntSushi
Copy link
Member

Let's do it! Thanks for the improvement @frewsxcv! @bors r+

@bors
Copy link
Contributor

bors commented Dec 6, 2017

📌 Commit f366227 has been approved by BurntSushi

@frewsxcv
Copy link
Member Author

frewsxcv commented Dec 6, 2017

@bors rollup

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Dec 6, 2017
…Sushi

Document behavior of `ptr::swap` with overlapping regions of memory.

Fixes rust-lang#44479.
bors added a commit that referenced this pull request Dec 6, 2017
Rollup of 7 pull requests

- Successful merges: #46136, #46378, #46431, #46483, #46495, #46502, #46512
- Failed merges:
@bors bors merged commit f366227 into rust-lang:master Dec 6, 2017
@frewsxcv frewsxcv deleted the frewsxcv-ptr-swap branch December 11, 2017 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

8 participants