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

Rc as rcref #10

Closed
wants to merge 5 commits into from
Closed

Rc as rcref #10

wants to merge 5 commits into from

Conversation

noamtashma
Copy link
Contributor

This pull request adds a feature allowing StaticRc and StaticRcRef to be frozen in order to temporarily create a new StaticRcRef that has the same "ratio" of a &mut T. Since the original is frozen, the overall ownership of the &mut T is still preserved. This can allow changing a value through separate StaticRcs stored in different parts while not having to constantly move them around.

Honestly I'm not sure how useful it is, because I haven't had time to check yet, but I have a feeling this can be used to improve some of the data structures that can currently be implemented using static_rc.

I'm pretty sure this is sound, but this does "allow" more than 1 overall ownership at a single time. But it isn't supposed to matter since only 1 overall ownership is usable at any single time. Perhaps you would like to put this under an experimental feature for now?

The pull request is currently missing tests, except the two doctests.

@matthieu-m
Copy link
Owner

This looks interesting overall, and I think that the reborrows are indeed safe.

As you mention, this should drastically increase the flexibility of the types, which looks quite useful.

I am not sure whether this would require an "experimental" flag, or not. It seems relatively tame (unlike lift, which is outright dodgy) and very much in line with existing functions which are not marked as such.

In the end, I am left only with nits:

  1. Vocabulary: I'd rather use "mutably borrowed" than "frozen". "frozen" doesn't capture the idea that the instance cannot be used; it conveys a sense of immutability to me, but immutable instances can be used after all (in read-only mode). Mutably borrowed is the technically correct term, if not as catchy, and anyone who understands borrowing understands the consequences of mutably borrowing something.
  2. Placement:
    • If you look at how the impl are laid out, you'll realize that pub methods come first, and private methods last, this is to allow the reader to see the public API first and foremost.
    • Furthermore, I would lift the methods accessing a single Rc/RcRef before the methods doing the more complicated split/join, so would place those "borrow" methods right after the adjust one, so that the methods go in order of complexity in a way.
  3. Documentation formatting: the documentation comments are formatted in a very specific style, that I would appreciate you followed too for consistency:
    • A single sentence summing up the method, followed by a blank line.
    • Optionally, a more thorough explanation of what the method does.
    • Then explicitly named sections: Warning, Safety, ... ending with an Example section.
  4. Testing, indeed. In particular, I would like to see compile-tests which ensure a compile-time error if the supposedly borrowed "originals" can be used in any way, eg with a call to get_ref.
    • In rcref.rs there's already a compile_tests module: you can follow the model there. Please place the test(s?) right after the existing one.
    • In rc.rs there's no compile_tests module yet, please introduce one before the other specialized compile-time tests (make sure it's doc-hidden) and put any test(s?) there.

Looking forward to getting this merged :)

@noamtashma
Copy link
Contributor Author

noamtashma commented Jan 31, 2022

I also think the names should be reconsidered. I would like the names to reflect the similarity between the functions.
I would rename as_rcref' to 'reborrow' as well, but it doesn't make sense because it isn't a reference itself. I would name it borrow` but 'borrow' conflicts with 'std::Borrow'.

Maybe borrow_as_rcref?

@matthieu-m
Copy link
Owner

Naming is hard :(

I find borrow_as_rcref() to be quite mouthful, while at the same time failing to indicate that it's a mutable borrow (generally indicated by a _mut in the method.

I do agree that aligning the names would be neat; I don't have much spare brain power right now, though, so I wouldn't hold back the PR on that. It's not stable, we can always change the names later if a better idea pops up.

@noamtashma
Copy link
Contributor Author

I believe that's it :)

@matthieu-m
Copy link
Owner

Don't panic!

I wanted to squash, rebase, and reword the commit message to be more inline with the style used by the other commit messages... and I couldn't find a way to do it from Github.

So I did it locally -- and painstakingly, I'm really no good at advanced workflows -- while managing to preserve attribution.

I didn't quite know how to update this PR from there, so I just pushed from my local checkout to master; and therefore this PR's code is in, even if this PR is closed.

Thanks for your work!

@matthieu-m matthieu-m closed this Feb 5, 2022
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