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 try_zeroed_vec and zeroed_vec #117

Merged
merged 3 commits into from
Jul 7, 2022
Merged

Add try_zeroed_vec and zeroed_vec #117

merged 3 commits into from
Jul 7, 2022

Conversation

fu5ha
Copy link
Collaborator

@fu5ha fu5ha commented Jul 4, 2022

So, I believe this is sound as it essentially exactly mimics the internal implementation of Vec::with_capacity except using alloc_zeroed instead. However, Vec::from_raw_parts_mut is scary so would like to get someone else to review it and make sure I'm not going crazy.

@fu5ha fu5ha requested a review from Lokathor July 4, 2022 14:22
@5225225
Copy link

5225225 commented Jul 4, 2022

I believe vec's allocation strategy is unspecified, so using from_raw_parts on something vec didn't allocate itself is questionable.

I'd do

let mut v = Vec::new();
v.try_reserve(length)?;
// somehow zero the vec, maybe make a zeroed item with T::zeroed() and write it to all items? or use `resize_with(T::zeroed)`
Ok(v)

@Noratrieb
Copy link
Contributor

Noratrieb commented Jul 4, 2022

I believe vec's allocation strategy is unspecified

Sounds like a bug in vecs documentation. Vec<T> should always be allocated using the global allocator (as it currently works with the unstable allocator API), so in my opinion this should be fine.

src/allocation.rs Show resolved Hide resolved
@@ -131,7 +151,7 @@ pub fn try_zeroed_slice_box<T: Zeroable>(
let slice =
unsafe { core::slice::from_raw_parts_mut(ptr as *mut T, length) };
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be core::ptr::slice_from_raw_parts_mut() to avoid creating an intermediate reference (also also reduces the amount of unsafe by one line).

Copy link
Owner

Choose a reason for hiding this comment

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

That's a possible change, but in this case the intermediate reference is fine, because the allocation is initialized anyway.

@fu5ha
Copy link
Collaborator Author

fu5ha commented Jul 4, 2022

I think try_zeroed_boxed_slice().into_vec() is the best strategy here :) Didn't realize into_vec existed on a boxed slice

@fu5ha fu5ha changed the title Add try_zeroed_vec and zeroed_vec, adjust impl of try_zeroed_slice_box Add try_zeroed_vec and zeroed_vec Jul 4, 2022
@fu5ha
Copy link
Collaborator Author

fu5ha commented Jul 7, 2022

@Lokathor I think this is a pretty easy change with the new strategy. Thoughts/hesitations?

@Lokathor Lokathor merged commit e612031 into main Jul 7, 2022
@Lokathor Lokathor deleted the zeroed-vec branch July 7, 2022 14:54
leod pushed a commit to leod/bytemuck that referenced this pull request Jun 3, 2023
* add `try_zeroed_vec` and `zeroed_vec`, adjust impl of `try_zeroed_slice_box`

* go back to returning an error rather than calling handle_alloc_error

* use boxed slice .into_vec instead :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants