refactor: remove bump field from ChunkIter#319
refactor: remove bump field from ChunkIter#319overlookmotel wants to merge 1 commit intofitzgen:mainfrom
bump field from ChunkIter#319Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to simplify ChunkIter by removing its redundant bump: PhantomData<&'a mut Bump> field, relying on ChunkRawIter to carry the lifetime instead.
Changes:
- Remove the
bump: PhantomData<&'a mut Bump>field fromChunkIter - Simplify
iter_allocated_chunksto constructChunkIterwith only itsrawiterator
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -2388,7 +2385,6 @@ impl<const MIN_ALIGN: usize> Bump<MIN_ALIGN> { | |||
| #[derive(Debug)] | |||
| pub struct ChunkIter<'a, const MIN_ALIGN: usize = 1> { | |||
| raw: ChunkRawIter<'a, MIN_ALIGN>, | |||
There was a problem hiding this comment.
Removing bump: PhantomData<&'a mut Bump<MIN_ALIGN>> from ChunkIter weakens the borrow the iterator holds to an immutable borrow via ChunkRawIter (ChunkRawIter currently uses PhantomData<&'a Bump<MIN_ALIGN>>). Since Bump has allocation APIs that take &self (interior mutability), callers can allocate while a safe ChunkIter is alive, which can invalidate the returned chunk slices and lead to UB. Keep the &'a mut Bump phantom in ChunkIter, or otherwise ensure the iterator type enforces exclusive access for the duration of iteration.
| raw: ChunkRawIter<'a, MIN_ALIGN>, | |
| raw: ChunkRawIter<'a, MIN_ALIGN>, | |
| bump: PhantomData<&'a mut Bump<MIN_ALIGN>>, |
| pub fn iter_allocated_chunks(&mut self) -> ChunkIter<'_, MIN_ALIGN> { | ||
| // Safety: Ensured by mutable borrow of `self`. | ||
| let raw = unsafe { self.iter_allocated_chunks_raw() }; | ||
| ChunkIter { | ||
| raw, | ||
| bump: PhantomData, | ||
| } | ||
| ChunkIter { raw } |
There was a problem hiding this comment.
The safety comment here (“Ensured by mutable borrow of self.”) becomes incorrect if ChunkIter no longer carries a PhantomData<&'a mut Bump<MIN_ALIGN>>: the iterator only keeps an immutable borrow, and Bump exposes mutation via &self (e.g., alloc, try_alloc_layout, etc.). This means allocations can occur while the safe iterator is alive, violating the safety preconditions for producing &'a [MaybeUninit<u8>].
|
Copilot's critique sounds reasonable! I'm not actually sure he's right, but keeping the |
Minor simplification.
It's unnecessary for
ChunkIterto contain aPhantomData<&'a mut Bump>because it also contains aChunkRawIter<'a, MIN_ALIGN>- and that containsPhantomData<&'a mut Bump>. This is sufficient to "hold" the lifetime for both iterators.