Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arbitrary borrow count #9

Closed
wants to merge 10 commits into from

Conversation

RustyYato
Copy link
Contributor

This allows you to borrow as many rw borrows as you want. (remove the limitation of 3)

@RustyYato
Copy link
Contributor Author

RustyYato commented Dec 24, 2019

TODO List

  • Document techniques used to get var-args like behavior
  • Add safety docs for all unsafe traits
  • Make macro more ergonomic to use (around missing deref coercions)

@RustyYato
Copy link
Contributor Author

RustyYato commented Dec 24, 2019

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)

@uazu
Copy link
Owner

uazu commented Dec 25, 2019

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.

@RustyYato
Copy link
Contributor Author

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.

@RustyYato
Copy link
Contributor Author

RustyYato commented Dec 25, 2019

Could you add as_ptr and as_mut_ptr methods to all of the *Cells. 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.

@uazu
Copy link
Owner

uazu commented Dec 25, 2019

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.

@RustyYato
Copy link
Contributor Author

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.

@uazu
Copy link
Owner

uazu commented Dec 25, 2019

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.

@RustyYato RustyYato mentioned this pull request Dec 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants