Messages in this thread | | | Date | Mon, 25 Sep 2023 16:11:51 +0000 | From | Benno Lossin <> | Subject | Re: [PATCH v2 2/2] rust: arc: remove `ArcBorrow` in favour of `WithRef` |
| |
On 25.09.23 18:02, Boqun Feng wrote: > On Mon, Sep 25, 2023 at 05:30:05PM +0200, Alice Ryhl wrote: >> On 9/25/23 17:17, Boqun Feng wrote: >>> On Mon, Sep 25, 2023 at 03:00:47PM +0000, Alice Ryhl wrote: >>>>>>> I'm concerned about this change, because an `&WithRef<T>` only has >>>>>>> immutable permissions for the allocation. No pointer derived from it >>>>>>> may be used to modify the value in the Arc, however, the drop >>>>>>> implementation of Arc will do exactly that. >>>>>> >>>>>> That is indeed a problem. We could put the value in an `UnsafeCell`, but >>>>>> that would lose us niche optimizations and probably also other optimizations. >>>>>> >>>>> >>>>> Not sure I understand the problem here, why do we allow modifying the >>>>> value in the Arc if you only have a shared ownership? >>>> >>>> Well, usually it's when you have exclusive access even though the value >>>> is in an `Arc`. >>>> >>>> The main example of this is the destructor of the `Arc`. When the last >>>> refcount drops to zero, this gives you exclusive access. This lets you >>>> run the destructor. The destructor requires mutable access. >>>> >>>> Another example would be converting the `Arc` back into an `UniqueArc` >>>> by checking that the refcount is 1. Once you have a `UniqueArc`, you can >>>> use it to mutate the inner value. >>>> >>>> Finally, there are methods like `Arc::get_mut_unchecked`, where you >>>> unsafely assert that nobody else is using the value while you are >>>> modifying it. We don't have that in our version of `Arc` right now, but >>>> we might want to add it later. >>>> >>> >>> Hmm.. but the only way to get an `Arc` from `&WithRef` is >>> >>> impl From<&WithRef<T>> for Arc<T> { >>> ... >>> } >>> >>> , and we clone `Arc` in the that function (i.e. copying the raw >>> pointer), so we are still good? >>> >> >> No, the raw pointer in the Arc was created from the immutable reference, so >> the raw pointer has the same restrictions as the immutable reference did. >> > > I see, this was the part I was missing. Thanks! > > Looks like the only fix is replacing `&WithRef<T>` with > `&UnsafeCell<WithRef<T>>`? But that's a bit wordy and I'm not sure > whether it's better than `ArcBorrow<'_, T>`...
It should be sufficient to change only the type of the field within `WithRef`, so no visible API change needed.
-- Cheers, Benno
| |