Messages in this thread Patch in this message | | | Date | Fri, 20 Jan 2023 16:44:42 -0800 | From | Boqun Feng <> | Subject | Re: [PATCH 4/7] rust: sync: introduce `ArcBorrow` |
| |
Hi,
On Mon, Jan 16, 2023 at 10:07:36PM +0000, Gary Guo wrote: > On Wed, 4 Jan 2023 17:06:50 +0100 > Emilio Cobos Álvarez <emilio@crisal.io> wrote: > > > Sorry for the drive-by comment, but maybe it saves some work. > > > > On 1/4/23 16:29, Wedson Almeida Filho wrote: > > > On Sat, 31 Dec 2022 at 19:43, Gary Guo <gary@garyguo.net> wrote: > > >> > > >> On Wed, 28 Dec 2022 06:03:43 +0000 > > >> Wedson Almeida Filho <wedsonaf@gmail.com> wrote: > > >>> +pub struct ArcBorrow<'a, T: ?Sized + 'a> { > > >>> + inner: NonNull<ArcInner<T>>, > > >>> + _p: PhantomData<&'a ()>, > > >>> +} > > >>> + > > >>> +impl<T: ?Sized> Clone for ArcBorrow<'_, T> { > > >>> + fn clone(&self) -> Self { > > >>> + *self > > >>> + } > > >>> +} > > >>> + > > >>> +impl<T: ?Sized> Copy for ArcBorrow<'_, T> {} > > >> > > >> Couldn't this just be derived `Clone` and `Copy`? > > > > > > Indeed. I'll send a v2 with this. > > > > I'm not sure this is true. Deriving will add the T: Copy and T: Clone > > bound, which I think is not what you want here. > > > > i.e., I assume you want an ArcBorrow to be Copy even if the underlying T > > is not. > > Thanks for pointing out, I neglected that. >
Somehow I run into this code again, and after a few thoughts, I'm wondering: isn't ArcBorrow just &ArcInner<T>?
I've tried the following diff, and it seems working. The better parts are 1) #[derive(Clone, Copy)] works and 2) I'm able to remove a few code including one "unsafe" in ArcBorrow::deref().
But sure, more close look is needed. Thoughts?
Regards, Boqun
> In this case: > > Reviewed-by: Gary Guo <gary@garyguo.net>
--------------------------------->8 diff --git a/rust/kernel/sync/arc.rs b/rust/kernel/sync/arc.rs index ff73f9240ca1..48f919878f44 100644 --- a/rust/kernel/sync/arc.rs +++ b/rust/kernel/sync/arc.rs @@ -185,7 +185,7 @@ impl<T: ?Sized> Arc<T> { // SAFETY: The constraint that the lifetime of the shared reference must outlive that of // the returned `ArcBorrow` ensures that the object remains alive and that no mutable // reference can be created. - unsafe { ArcBorrow::new(self.ptr) } + ArcBorrow(unsafe { self.ptr.as_ref() }) } } @@ -298,52 +298,25 @@ impl<T: ?Sized> From<Pin<UniqueArc<T>>> for Arc<T> { /// let obj = Arc::try_new(Example { a: 10, b: 20 })?; /// obj.as_arc_borrow().use_reference(); /// ``` -pub struct ArcBorrow<'a, T: ?Sized + 'a> { - inner: NonNull<ArcInner<T>>, - _p: PhantomData<&'a ()>, -} +#[derive(Clone, Copy)] +pub struct ArcBorrow<'a, T: ?Sized>(&'a ArcInner<T>); // This is to allow [`ArcBorrow`] (and variants) to be used as the type of `self`. impl<T: ?Sized> core::ops::Receiver for ArcBorrow<'_, T> {} // This is to allow `ArcBorrow<U>` to be dispatched on when `ArcBorrow<T>` can be coerced into // `ArcBorrow<U>`. -impl<T: ?Sized + Unsize<U>, U: ?Sized> core::ops::DispatchFromDyn<ArcBorrow<'_, U>> - for ArcBorrow<'_, T> +impl<'a, T: ?Sized + Unsize<U>, U: ?Sized> core::ops::DispatchFromDyn<ArcBorrow<'a, U>> + for ArcBorrow<'a, T> { } -impl<T: ?Sized> Clone for ArcBorrow<'_, T> { - fn clone(&self) -> Self { - *self - } -} - -impl<T: ?Sized> Copy for ArcBorrow<'_, T> {} - -impl<T: ?Sized> ArcBorrow<'_, T> { - /// Creates a new [`ArcBorrow`] instance. - /// - /// # Safety - /// - /// Callers must ensure the following for the lifetime of the returned [`ArcBorrow`] instance: - /// 1. That `inner` remains valid; - /// 2. That no mutable references to `inner` are created. - unsafe fn new(inner: NonNull<ArcInner<T>>) -> Self { - // INVARIANT: The safety requirements guarantee the invariants. - Self { - inner, - _p: PhantomData, - } - } -} - impl<T: ?Sized> From<ArcBorrow<'_, T>> for Arc<T> { fn from(b: ArcBorrow<'_, T>) -> Self { // SAFETY: The existence of `b` guarantees that the refcount is non-zero. `ManuallyDrop` // guarantees that `drop` isn't called, so it's ok that the temporary `Arc` doesn't own the // increment. - ManuallyDrop::new(unsafe { Arc::from_inner(b.inner) }) + ManuallyDrop::new(unsafe { Arc::from_inner(b.0.into()) }) .deref() .clone() } @@ -353,9 +326,7 @@ impl<T: ?Sized> Deref for ArcBorrow<'_, T> { type Target = T; fn deref(&self) -> &Self::Target { - // SAFETY: By the type invariant, the underlying object is still alive with no mutable - // references to it, so it is safe to create a shared reference. - unsafe { &self.inner.as_ref().data } + &self.0.data } }
| |