Use phantomdata in CopyOnDrop #1032
Conversation
|
Also it might be worth unifying CopyOnDrop with the ManuallyDrops |
src/slice/quicksort.rs
Outdated
| use std::mem::{self, MaybeUninit}; | ||
| use std::ptr; | ||
|
|
||
| /// When dropped, copies from `src` into `dest`. | ||
| struct CopyOnDrop<T> { | ||
| struct CopyOnDrop<'src, 'dest, T> { |
There was a problem hiding this comment.
Is it useful to have two lifetimes? They're both covariant, while we're overall invariant in T itself.
There was a problem hiding this comment.
I decided to go for full accuracy in the phantomdata, but we could technically have just one here
There was a problem hiding this comment.
I guess in a few places you're specifying explicit lifetimes parameters, otherwise NLL would probably tighten it to the life of CopyOnDrop itself.
There was a problem hiding this comment.
Would you like me to change it or leave it as is?
There was a problem hiding this comment.
Let's change to one for simplicity, thanks.
There was a problem hiding this comment.
Also I just realized that the first lifetime is unbound anyway. Going to fix this with a constructor, but annoyingly there's no easy way to pass down lifetimes for raw pointers
There was a problem hiding this comment.
How does this look? I could accept a third &mut ptr that ensures that dest is also factored in to the lifetime calculation but for now this seems to work.
Really wish there were a "raw pointer with a lifetime" type that had all the right methods but also the lifetime (but none of the validity constraints)
| use std::mem::{self, MaybeUninit}; | ||
| use std::ptr; | ||
|
|
||
| /// When dropped, copies from `src` into `dest`. | ||
| struct CopyOnDrop<T> { | ||
| struct CopyOnDrop<'a, T> { | ||
| src: *const T, |
There was a problem hiding this comment.
Looking at where we create CopyOnDrop, I think this could just be &'a T?
(But the marker is still useful for invariance in T.)
Or I guess this is where you were suggesting unifying ManuallyDrop... TBH it feels a little uncomfortable that we're even creating a live T copy for ManuallyDrop::new in the first place. We could avoid that with src: MaybeUninit<T>, initialized by copy from a source pointer. The lifetime would just become 'dest, turbofish-constrained as 'v when we create it. What do you think?
There was a problem hiding this comment.
I don't know, we're making T copies within v: &mut [T] all over the place, so maybe that MaybeUninit idea doesn't get much closer to real caution here.
There was a problem hiding this comment.
Yeah I think it's definitely cleaner to unify it with ManuallyDrop but it's not safer or anything. I don't want to do the broader refactor here though, I was trying to get a small defense in depth win.
|
bors r+ |
There is a struct with a destructor borrowing from a local, it would be good to have some lifetimes protecting it from doing the wrong thing.
mergesort.rs also has this type but it's constructing it from raw pointers and I didn't want to infect everything with lifetimes. It's less of a problem there since it's not borrowing from locals.