diff --git a/src/vec/vec-alloc.md b/src/vec/vec-alloc.md index b301fc6..82f637e 100644 --- a/src/vec/vec-alloc.md +++ b/src/vec/vec-alloc.md @@ -187,10 +187,20 @@ impl Vec { assert!(new_layout.size() <= isize::MAX as usize, "Allocation too large"); let new_ptr = if self.cap == 0 { + // Safety: alloc::alloc requires new_layout to be non-zero size. + // + // Since we would have panicked in new for a ZST, this would only + // be the case if the value passed to Layout::array() was non-zero + // which is never the case. unsafe { alloc::alloc(new_layout) } } else { let old_layout = Layout::array::(self.cap).unwrap(); let old_ptr = self.ptr.as_ptr() as *mut u8; + // Safety: alloc::realloc requires old_ptr be allocated with old_layout, + // and new_layout.size() not be zero or overflow to above usize::MAX + // + // new_layout.size() being zero is covered by the above safety comment + // and the overflow is covered by the "Allocation too large" assertion above. unsafe { alloc::realloc(old_ptr, old_layout, new_layout.size()) } }; diff --git a/src/vec/vec-layout.md b/src/vec/vec-layout.md index c1c1afc..565560a 100644 --- a/src/vec/vec-layout.md +++ b/src/vec/vec-layout.md @@ -46,16 +46,38 @@ use std::ptr::NonNull; use std::marker::PhantomData; pub struct Vec { + // Invariant 1: Either cap is 0, or ptr is a Layout::array of cap T's + // Invariant 2: from ptr onwards is `len` amount of initialized T's ptr: NonNull, cap: usize, len: usize, _marker: PhantomData, } +// Safety: Semantically we own T's, so Send if T: Send, Sync if T: Sync is correct unsafe impl Send for Vec {} unsafe impl Sync for Vec {} # fn main() {} ``` +## Safety Comments and Invariants + +Before going on, it's worth explaining the comment above the impls of +`Send`/`Sync` for `Vec`. + +These are known as safety comments, and are good practice to get into doing if +you are writing any unsafe code. They help any reader understand why the +`unsafe` blocks are sound. + +Additionally, making yourself write them encourages you to think harder about +the safety of your code, possibly leading you to discover bugs. + +There is [a clippy lint][undocumented_unsafe_blocks] to warn when you forget to +do this, which you may wish to enable. + +The rest of the section on implementing `Vec` will use safety comments to +explain why its use of `unsafe` is correct. + [ownership]: ../ownership.html [NonNull]: ../../std/ptr/struct.NonNull.html +[undocumented_unsafe_blocks]: https://rust-lang.github.io/rust-clippy/master/index.html#undocumented_unsafe_blocks diff --git a/src/vec/vec-push-pop.md b/src/vec/vec-push-pop.md index 2a4e016..be3d84b 100644 --- a/src/vec/vec-push-pop.md +++ b/src/vec/vec-push-pop.md @@ -22,6 +22,9 @@ to the 0th index. So we should offset by the old len. pub fn push(&mut self, elem: T) { if self.len == self.cap { self.grow(); } + // Safety: cap must be more than len, since we grew the vec. + // Therefore, self.ptr must point to at least self.len + 1 items, which + // means self.ptr.as_ptr().add(self.len) is in-bounds of the allocation. unsafe { ptr::write(self.ptr.as_ptr().add(self.len), elem); } @@ -50,6 +53,9 @@ pub fn pop(&mut self) -> Option { } else { self.len -= 1; unsafe { + // Safety: By invariant 1, 2 on vec, the pointer here is + // in bounds and points to a valid T. We decrease the len + // so that we won't return it twice. Some(ptr::read(self.ptr.as_ptr().add(self.len))) } }