Skip to content

slice-dst: Add example for SliceWithHeader #70

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MattesWhite
Copy link

This PR is the result of the discussion in #69 .

It adds an example for the slice-dst crate and demonstrates how a custom Vec can be implemented.

Copy link
Owner

@CAD97 CAD97 left a comment

Choose a reason for hiding this comment

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

The example looks good modulo one tricky soundness pitfall (inline) that I want to be sure we aren't accidentally leading users into.


/// Our [`Vec`] implementation.
///
/// _Note:_ In contrast to [`std::vec::Vec`] this stores its length on the heap.
Copy link
Owner

Choose a reason for hiding this comment

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

suggestion: use erasable::Thin and store Thin<Box<HeapData<T>>>, so the pointer is thin (single-usize-big) and length/capacity are both exclusively on the heap. SliceWithHeader stores its (slice) length inline to support erasing. (The eventual custom derive will omit this.)

Copy link
Author

Choose a reason for hiding this comment

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

I'm a little bit reluctant to introduce erasable::Thin here. It addresses another issue and is from another crate. I would prefer to stick to an honorable mention here and put another example in the erasable crate.

}
fn iter(&self) -> impl Iterator<Item = &T> {
self.0.slice.iter().take(self.len()).map(|t| unsafe {
// Safe as only the initialized elements are iterated.
Copy link
Owner

@CAD97 CAD97 Apr 16, 2021

Choose a reason for hiding this comment

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

issue: unfortunately, this isn't sound as written, because anyone can use RefCast::ref_cast to convert between &MySlice and &HeapData, and change the header, changing how many initialized items are believed to exist. Or... it would be unsound if you added a DerefMut impl for MyVec, anyway. Having an example that's only sound due to a technicality is probably not the best idea 😄

suggestion: there are a few ways to fix this:

  • (lazy) note explicitly that access to &mut MySlice<_> would be unsound, due to the above;
  • (simple) just wrap [T] instead of HeapData<T> for MySlice<T>, as it doesn't need access to any of the other heap data (and is already a fat pointer, though using Thin could avoid this);
  • (chore) just inline the transmute usage that ref-cast abstracts away; or
  • (funky) use the priv-in-pub trick discussed in Support private casts dtolnay/ref-cast#9 to get an effectively private cast.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you very much for pointing this out. I chose the chore option with a detailed // SAFETY: comment. I hope this is okay for you.

@CAD97
Copy link
Owner

CAD97 commented Apr 16, 2021

Feel free to ignore the clippy warnings CI reported; I fixed them in #72.

The implementation of a 'cast trait' opened the opportunity to violate
`MyVec`'s contract, making the implementation unsound.
Keeping Cargo.lock in git often causes unnecessary conflicts when
merging branches.
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.

2 participants