Skip to content

Improve the example for ptr::copy #77385

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
Oct 2, 2020
Merged

Improve the example for ptr::copy #77385

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
20202da
Improve the example for ptr::copy
scottmcm Oct 1, 2020
e58f3d3
Things are only moved if non-copy
scottmcm Oct 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion library/core/src/intrinsics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1901,11 +1901,21 @@ pub unsafe fn copy_nonoverlapping<T>(src: *const T, dst: *mut T, count: usize) {
/// ```
/// use std::ptr;
///
/// /// # Safety:
Copy link
Contributor

@pickfire pickfire Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
/// /// # Safety:
/// /// # Safety
/// ///

We usually have a space after Safety for heading and don't have : at the end.

/// /// * `ptr` must be correctly aligned for its type and non-zero.
/// /// * `ptr` must be valid for reads of `elts` contiguous objects of type `T`.
Copy link
Contributor

@pickfire pickfire Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I never heard of "contiguous objects"? Is this perhaps a new term? Also, s/of/for.

Suggested change
/// /// * `ptr` must be valid for reads of `elts` contiguous objects of type `T`.
/// /// * `ptr` must be valid for reads of `elts` contiguous objects for type `T`.

Copy link
Member

@jyn514 jyn514 Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Try ctrl+f 'contiguous' on https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html#guarantees

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, contiguous is there but I never heard of contiguous objects. Try rg 'contiguous objects' in rust or https://github.com/rust-lang/rust/search?q=%22contiguous+objects%22

/// /// * Those elements must not be used after calling this function unless `T: Copy`.
Copy link
Contributor

@pickfire pickfire Oct 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Is this required? Is it only limited for Copy?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, if these are not Copy you'll have a double-free. ptr::read does not drop or move the source buffer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, I guess that's true but should this be part of safety? I think it could be but I wonder if this should be here.

/// # #[allow(dead_code)]
/// unsafe fn from_buf_raw<T>(ptr: *const T, elts: usize) -> Vec<T> {
/// let mut dst = Vec::with_capacity(elts);
/// dst.set_len(elts);
///
/// // SAFETY: Our precondition ensures the source is aligned and valid,
/// // and `Vec::with_capacity` ensures that we have usable space to write them.
/// ptr::copy(ptr, dst.as_mut_ptr(), elts);
///
/// // SAFETY: We created it with this much capacity earlier,
/// // and the previous `copy` has initialized these elements.
/// dst.set_len(elts);
/// dst
/// }
/// ```
Expand Down