Repository navigation
feat: proper conversions - #791
alejandro-vaz wants to merge 30 commits into
Conversation
|
reminder: fix proptest and warnings above |
|
Hey, shouldn't we break it into sub issues. As of now, I think it's too many breaking changes in a single PR |
|
honestly it all is intertwined so I'm not sure I can to complete conversions I had to expand the I honestly prefer big scoped PRs over micro-ones because it retains my flow state better |
|
No problem, let's just keep at it |
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
This was a really difficult PR to review. It's really big and it seems to mix functional changes with stylistic changes (like the try_from implementation, or renaming parameters/variables), so I had to read it really closely to figure out if anything was actually changing. Please find ways to keep those changes separate in the future.
| fn construct( | ||
| pointer: Option<NonNull<[MaybeUninit<Self::Item>]>>, | ||
| length: usize, | ||
| allocator: Self::Heap | ||
| ) -> Self::Type { | ||
| if let Some(pointer) = pointer { | ||
| unsafe { | ||
| Self::Type::from_raw_parts_in( | ||
| pointer.as_ptr().cast(), | ||
| length, | ||
| pointer.len(), | ||
| allocator | ||
| ) | ||
| } |
There was a problem hiding this comment.
I am worried that this method is marked safe because the docs for https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.from_raw_parts_in mention a bunch of reasons that's unsafe and those conditions do not appear to be checked anywhere.
There was a problem hiding this comment.
this is for internal use, but yes, it should be marked unsafe now that I think about it
I'll change it
| fn from(value: Cow<'_, [Item]>) -> Self { | ||
| match value { | ||
| Cow::Owned(mut vector) => unsafe { | ||
| Self::from_raw_parts(vector.as_mut_ptr(), vector.len(), vector.capacity()) |
There was a problem hiding this comment.
Why is this safe? from_raw_parts says it's for creating a smallvec from another smallvec, and that's not necessarily what is happening here.
There was a problem hiding this comment.
you are right, I've checked the invariants, we need #817 first
|
now awaiting #817 |
this remakes and generalizes conversions to comply fully with the allocator API and adds conversions to and from
Cow