Closed RustyYato closed 4 years ago
TODO List
unsafe traits
I'm not sure how to improve the ergonomics. Right now, you must pass it a reference to a *Cell
to the corresponding *CellOwner
(even through the macro), but this doesn't play too well with deref coercions like the current rw2
and rw3
methods do.
The best that I could think of is adding an AsRef
bound, but that seems like a bad idea unless we somehow cache results. If we don't cache results of AsRef
, we would have to depend on AsRef
returning a stable address which is a really bad idea. (It is possible to cache the results of AsRef
, but it would make the trait bounds for *CellOwner::rw_generic
ridiculous)
Thanks for the PR. I know you haven't quite finished yet, but my first impression is that this makes things a lot more complicated.
I've tried to keep things as simple as possible for an unsafe-code reviewer. I'm aware that companies will often want to review crate unsafety themselves, and if their reviewer understands what is going on, they will accept it -- otherwise not. So I have to keep things simple if I want the code to be used. Your code seems to have a lot more unsafe
keywords to check, plus more macro and type magic to figure out compared to the original.
Also, I reckoned I could see uses for rw2 (e.g. swapping values between two items), maybe for rw3, but I couldn't think of cases for more, so I stopped at 3. After a certain number (probably around 3 or 4) the caller is likely to be dealing with a dynamic number, which requires a different approach. I think it would be good to have a motivating example use-case for needing 4 or more mutable borrows.
I think if this was neat and an "easy win", then definitely I'd go for it. But it's looking like it's creating more problems for the crate than it is solving right now. Let me know what you think.
I've tried to keep things as simple as possible for an unsafe-code reviewer. I'm aware that companies will often want to review crate unsafety themselves, and if their reviewer understands what is going on, they will accept it -- otherwise not.
That is a very valid concern. A lot of the new unsafe
right now is self-inflicted, as in unsafe traits
and unsafe impl
to go along with it. But I don't think they need to be unsafe
now that I have sealed those traits.
I used unsafe
here more as a way to warn that this controls unsafe
somewhere else. For example, ValidateUniqueness
controls access to LoadValues
which is has the unsafe method load_values
's precondition to check.
I'll see how I can simplify this code.
After a certain number (probably around 3 or 4) the caller is likely to be dealing with a dynamic number, which requires a different approach
If you want a dynamic number of cells, they will all have to be the exact same type. However, this would greatly simplify the code. If you want to go in this direction, I can send a PR sometime this week for that.
Could you add as_ptr
and as_mut_ptr
methods to all of the *Cell
s. This would allow me to create a seperate crate that extends qcell
to allow for an arbitrary number of *Cell
. If it is fine, I can send a small PR with that this week.
Edit: Also an fn is_owned_by(&QCell, &QCellOwner) -> bool
for QCell
, to check if a QCellOwner
owns a given QCell
.
What I meant by a dynamic number is that you'd probably want to not borrow them all at the same time, but borrow them in subsets, e.g. borrow pairs at a time. If you borrow them all at the same time, checks go up as O(N^2). If I had an example use-case of how larger numbers of simultaneous borrows would be needed, it would make more sense. Using multiple owners is also an option for TCell/TLCell, although less convenient for LCell (lifetimes) or QCell (memory overhead).
Yes, I'm certainly up for adding more methods to allow you to create the functionality in another crate. Are you proposing to send me a PR for this? Or I could do it.
Ok, that makes sense, I'll make a PR for the extra functionality and split this into it's own crate.
Using multiple owners is also an option for TCell/TLCell
No, I thought that you are only allowed to use one owner per cell for it's entire lifetime. That's how it looks right now at least.
Thanks.
Yes, it's just one owner per cell. But if you have a situation where you need to borrow say 4 cells at the same time, if it can be arranged that they have different owners then that all works out fine.
This allows you to borrow as many
rw
borrows as you want. (remove the limitation of 3)