From b3c7268c11be6ef4252507211305e85b181b223a Mon Sep 17 00:00:00 2001 From: Joshua Simmons Date: Sat, 12 Nov 2022 00:50:53 +0100 Subject: [PATCH] Tidy some lints and documentation --- narcissus-core/src/fixed_vec.rs | 2 +- narcissus-core/src/manual_arc.rs | 56 ++++++++++++++++++-------------- narcissus-core/src/rand.rs | 2 +- narcissus-gpu/src/lib.rs | 12 +++++++ narcissus-gpu/src/vulkan.rs | 2 +- 5 files changed, 47 insertions(+), 27 deletions(-) diff --git a/narcissus-core/src/fixed_vec.rs b/narcissus-core/src/fixed_vec.rs index 7de170f..c9d3304 100644 --- a/narcissus-core/src/fixed_vec.rs +++ b/narcissus-core/src/fixed_vec.rs @@ -96,7 +96,7 @@ impl FixedVec { /// /// # Safety /// - /// - `new_len` must be less than or equal to [`capacity()`]. + /// - `new_len` must be less than or equal to [`Self::capacity()`]. /// - The elements at `old_len..new_len` must be initialized. #[inline] pub unsafe fn set_len(&mut self, new_len: usize) { diff --git a/narcissus-core/src/manual_arc.rs b/narcissus-core/src/manual_arc.rs index 1ab61b4..ea168bb 100644 --- a/narcissus-core/src/manual_arc.rs +++ b/narcissus-core/src/manual_arc.rs @@ -43,13 +43,14 @@ impl Inner { /// A thread-safe reference-counting pointer with manual management. /// -/// The type [`ManualArc`] provides shared ownership of a value of type T, allocated in the heap. Invoking clone on -/// [`ManualArc`] produces a new [`ManualArc`] instance, which points to the same allocation on the heap as the source -/// [`ManualArc`], while increasing a reference count. +/// The type [`ManualArc`] provides shared ownership of a value of type T, allocated in the heap. +/// Invoking clone on [`ManualArc`] produces a new [`ManualArc`] instance, which points to the same +/// allocation as the source, while increasing a reference count. /// -/// Before a [`ManualArc`] is dropped, the [`ManualArc::release`] method must be called. [`ManualArc::release`] will -/// return the variant [`Release::Shared`] if there are other references outstanding, or [`Release::Unique`] with the -/// object being released if the release operation removes the last reference. +/// Before a [`ManualArc`] is dropped, the [`ManualArc::release`] method must be called. +/// [`ManualArc::release`] will return the variant [`Release::Shared`] if there are other references +/// outstanding, or [`Release::Unique`] with the contained object if the release operation removes +/// the final reference. pub struct ManualArc { ptr: Option>>, phantom: PhantomData>, @@ -68,21 +69,24 @@ impl ManualArc { } } - /// Returns the variant [`Release::Shared`] if there are other [`ManualArc`] objects alive that reference the - /// object that this instance does, or [`Release::Unique`] with the previously contained object if the release - /// operation removes the last reference. + /// Consumes `self`, decrementing the reference count. /// - /// Must be explicitly called to drop [`ManualArc`] instances. Dropping implicitly or explicitly without calling - /// this function will panic. + /// Returns the variant [`Release::Shared`] if there are other [`ManualArc`] instances still + /// holding references to the same object, or [`Release::Unique`] with the previously contained + /// object if the release operation removes the last reference. + /// + /// Must be explicitly called to drop [`ManualArc`] instances. Dropping implicitly or explicitly + /// without calling this function will panic. pub fn release(mut self) -> Release { #[cold] #[inline(never)] unsafe fn release_slow(ptr: NonNull>) -> T { - // Ref-counting operations imply a full memory barrier on x86, but not in general. So insert an acquire - // barrier on the slow path here to ensure all modifications to inner are visible before we call drop. + // Ref-counting operations imply a full memory barrier on x86, but not in general. So + // insert an acquire barrier on the slow path to ensure all modifications to inner are + // visible before we call drop. std::sync::atomic::fence(Ordering::Acquire); - // Safety: Was created by Box::leak in the constructor, so it's valid to re-create a box here. + // Safety: Was created by Box::leak in the constructor, so it's valid to recreate a box. let mut inner = Box::from_raw(ptr.as_ptr()); // extract the value from the container so we can return it. let value = ManuallyDrop::take(&mut inner.value); @@ -92,8 +96,9 @@ impl ManualArc { value } - // Safety: `release` consumes self, so it's impossible to call twice. Therefore ptr is always valid since - // `release` is the only thing which invalidates it. + // Safety: `release` consumes `self` so it's impossible to call twice on the same instance, + // release is also the only function able to invalidate the pointer. Hence the pointer is + // always valid here. unsafe { // Replace ptr with None so that the drop function doesn't panic let ptr = std::mem::replace(&mut self.ptr, None); @@ -102,8 +107,8 @@ impl ManualArc { if inner.decr_strong() { Release::Shared } else { - // We have released the last reference to this inner, so we need to free it and return the contained - // value. + // We have released the last reference to this inner, so we need to free it and + // return the contained value. let value = release_slow(ptr); Release::Unique(value) } @@ -119,11 +124,13 @@ impl Default for ManualArc { impl Clone for ManualArc { fn clone(&self) -> Self { - // Safety: Inner is valid whilever we have a valid [`ManualArc`] outside of the [`ManualArc::release`] function. - let ptr = unsafe { self.ptr.unwrap_unchecked() }; - let inner = unsafe { ptr.as_ref() }; - inner.incr_strong(); - Self::from_inner(ptr) + // Safety: Inner is valid whilever we have a valid `ManualArc`, and so long as we are outside + // the `release` function. + unsafe { + let ptr = self.ptr.unwrap_unchecked(); + ptr.as_ref().incr_strong(); + Self::from_inner(ptr) + } } } @@ -138,7 +145,8 @@ impl Drop for ManualArc { impl Deref for ManualArc { type Target = T; - // Safety: Inner is valid whilever we have a valid [`ManualArc`] outside of the [`ManualArc::release`] function. + // Safety: Inner is valid whilever we have a valid `ManualArc`, and so long as we are outside + // the `release` function. #[inline(always)] fn deref(&self) -> &Self::Target { unsafe { self.ptr.unwrap_unchecked().as_ref() } diff --git a/narcissus-core/src/rand.rs b/narcissus-core/src/rand.rs index e262acf..e6eccbe 100644 --- a/narcissus-core/src/rand.rs +++ b/narcissus-core/src/rand.rs @@ -33,7 +33,7 @@ impl Pcg64 { /// Generates a uniformly distributed random number in the range `0..upper_bound` /// - /// Based on https://github.com/apple/swift/pull/39143/commits/87b3f607042e653a42b505442cc803ec20319c1c + /// Based on #[inline] #[must_use] pub fn next_bound_u64(&mut self, upper_bound: u64) -> u64 { diff --git a/narcissus-gpu/src/lib.rs b/narcissus-gpu/src/lib.rs index b235c2e..86263f1 100644 --- a/narcissus-gpu/src/lib.rs +++ b/narcissus-gpu/src/lib.rs @@ -274,7 +274,19 @@ pub trait Device { ); fn destroy_pipeline(&self, frame_token: &FrameToken, pipeline: Pipeline); + /// Map the given buffer in its entirety to system memory and return a pointer to it. + /// + /// # Safety + /// + /// `buffer` must be host mappable. unsafe fn map_buffer(&self, buffer: Buffer) -> *mut u8; + + /// Unmap from system memory a buffer previously mapped. + /// + /// # Safety + /// + /// This will invalidate the pointer returned previously from `map_buffer`, so there must not be + /// any remaining references derived from that address. unsafe fn unmap_buffer(&self, buffer: Buffer); fn acquire_swapchain( diff --git a/narcissus-gpu/src/vulkan.rs b/narcissus-gpu/src/vulkan.rs index 3633a1f..76413c7 100644 --- a/narcissus-gpu/src/vulkan.rs +++ b/narcissus-gpu/src/vulkan.rs @@ -642,7 +642,7 @@ impl<'app> VulkanDevice<'app> { } }); - let descriptor_pool_pools = GpuConcurrent::new(|| vk::DescriptorPool::null()); + let descriptor_pool_pools = GpuConcurrent::new(vk::DescriptorPool::null); UnsafeCell::new(VulkanFrame { command_buffer_pools, -- 2.49.0