On 15.08.24 16:09, Danilo Krummrich wrote: > On Thu, Aug 15, 2024 at 01:39:05PM +0000, Benno Lossin wrote: >> On 15.08.24 15:33, Danilo Krummrich wrote: >>> On Thu, Aug 15, 2024 at 02:34:50PM +0200, Alice Ryhl wrote: >>>> On Thu, Aug 15, 2024 at 2:33 PM Danilo Krummrich <dakr@xxxxxxxxxx> wrote: >>>>> >>>>> On Thu, Aug 15, 2024 at 11:20:32AM +0200, Alice Ryhl wrote: >>>>>> On Thu, Aug 15, 2024 at 4:52 AM Danilo Krummrich <dakr@xxxxxxxxxx> wrote: >>>>>>> >>>>>>> On Wed, Aug 14, 2024 at 12:32:15PM -0700, Boqun Feng wrote: >>>>>>>> Hi Danilo, >>>>>>>> >>>>>>>> I'm trying to put your series on rust-dev, but I hit a few conflicts due >>>>>>>> to the conflict with `Box::drop_contents`, which has been in rust-dev >>>>>>>> for a while. And the conflict is not that trivial for me to resolve. >>>>>>>> So just a head-up, that's a requirement for me to put it on rust-dev for >>>>>>>> more tests from my end ;-) >>>>>>> >>>>>>> I rebased everything and you can fetch them from [1]. >>>>>>> >>>>>>> I resolved the following conflicts: >>>>>>> >>>>>>> - for `Box`, implement >>>>>>> - `drop_contents` >>>>>>> - `manually_drop_contents` [2] >>>>>> >>>>>> Not sure I like this name. It sounds like something that runs the >>>>>> destructor, but it does the exact opposite. >>>>> >>>>> I thought it kinda makes sense, since it's analogous to `ManuallyDrop::new`. >>>>> >>>>> What about `Box::forget_contents` instead? >>>> >>>> One option is `into_manually_drop`. This uses the convention of using >>>> the `into_*` prefix for conversions that take ownership of the >>>> original value. >>> >>> The signature of the current `Box::manually_drop_contents` is the same as for >>> `Box::drop_contents`, namely >>> `fn manually_drop_contents(this: Self) -> Box<MaybeUninit<T>, A>`. >>> >>> `into_manually_drop` seems misleading for for returning a >>> `Box<MaybeUninit<T>, A>`. >>> >>> I still think `forget_contents` hits it quite well. Just as `drop_contents` >>> drops the value, `forget_contents` makes the `Box` forget the value. >> >> I think `forget_contents` sounds good. Can you please add some more docs >> to that function though? Like an example and change "Manually drops the >> contents, but keeps the allocation" to "Forgets the contents (does not >> run the destructor), but keeps the allocation.". > > I can't really think of a good real world example other than moving out of a > `Box`, can you? Otherwise, maybe it just shouldn't be public? Oh I thought you had a user for that function. In that case making it private makes a lot of sense. Also, `drop_contents` can be implemented with `forget_contents`, but that might be a good follow up as a good-first-issue. >> Another thing that I spotted while looking at the patch, `move_out` >> doesn't need the `transmute_copy`, you should be able to just call >> `read` on the pointer. > > While technically it's the same I thought `transmute_copy` is considered better, > since it has less stict safety requirements? I would avoid `transmute_copy` as much as possible, transmuting signifies to me that you somehow need to change the type and `transmute_copy` means that the compiler isn't even able to figure out that the two types have the same size (they are allowed to have different size [only `Src` larger than `Dst` though], but I most often have used this with generics where it was guaranteed to be the same type, but the compiler failed to notice it.). > For `transmute_copy` we only need to say, that dst has the same type as src, > whereas for `read` the pointer must be valid for reads, properly aligned and > point to an initialized value. You can input a reference, so you get pointer validity for free, though you still need to put that in the SAFETY comment. --- Cheers, Benno
