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

Added Arc::try_unique #23844

Merged
merged 1 commit into from
Apr 2, 2015
Merged

Added Arc::try_unique #23844

merged 1 commit into from
Apr 2, 2015

Conversation

kvark
Copy link
Contributor

@kvark kvark commented Mar 29, 2015

While trying to implement parallel ECS processing, I stumbled upon the need to mutate Arc contents. The only existed method that allowed that was make_unique, but it has issues:

  • it may clone the data as if nothing happened, where the program may just need to crash
  • it forces Clone bound, which I don't have

The new try_unique allows accessing the contents mutably without Clone bound and error out if the pointer is not unique.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Thanks for the PR! In the past we've generally been pretty hesitant about adding inherent methods on smart pointers, however as they will shadow all other methods of the same name. In the past we've requested that extra functionality for smart pointers go as top-level functions as well.

Also, we do seem to definitely have a bit of a sprawling story for extra functionality on smart pointers, there's an interesting subset of functionality between std::rc and std::arc. I'm not sure what the best story is here for these functions, and we may want to hold off on landing new features until we've had time to reconsider the extra functionality. That being said though these are all unstable so I'm also ok landing in the interim (despite that they may change slightly).

cc @aturon, do you have thoughts on adding apis such as this?

@kvark
Copy link
Contributor Author

kvark commented Mar 30, 2015

@alexcrichton thanks for the info!

May I wonder why top-level functions are preferred for smart pointers instead of the member functions?

I'd be happy to use the existing functions without introducing more, but as I said there is no way to get the mutable contents at the moment without Clone bound, which I don't have.

As for the intersection, for gfx-rs we'll need some sort of abstraction over Rc/Arc in order to support single-threaded/multi-threaded resource management without extra overhead (gfx-rs/gfx#636). AFAIK, that does require HKT, but we can prepare for it now by introducing a common trait that both Rc and Arc implement.

@alexcrichton
Copy link
Member

May I wonder why top-level functions are preferred for smart pointers instead of the member functions?

Right now if a smart pointer P defines the method foo, then it is impossible to call the method foo on a type T if you say something like:

let ptr: P<T> = ...;
ptr.foo();

This is because the receiver's methods trump the Deref'd type's methods, so foo will always be dispatched to P instead of T. This can be worked around with UFCS, however, so this isn't as pressing as it was before!

@kvark
Copy link
Contributor Author

kvark commented Mar 30, 2015

Oh, thanks, I get this now!
In case of such a conflict, one can always do ptr.deref_mut().foo(), right?

@alexcrichton
Copy link
Member

You can indeed! I also forgot, but you can also do (*ptr).foo() to resolve the conflict as well.

@kvark
Copy link
Contributor Author

kvark commented Mar 31, 2015

Just to clarify - are we waiting for me to change the methods to be global, or are we blocked by @aturon to look into it?

@aturon
Copy link
Member

aturon commented Mar 31, 2015

@kvark Sorry for the delay. I'm happy to add a top-level experimental APIs like this.

I'd like to suggest a single method unique that produces an Option<&mut T>. You can recover is_unique through unique().is_some().

@kvark
Copy link
Contributor Author

kvark commented Mar 31, 2015

@aturon Right, I only added is_unique as a private helper, with the idea that users do try_unique().is_some() if they need it.

I'll update the PR shortly. Thanks for having a look!

@kvark
Copy link
Contributor Author

kvark commented Apr 1, 2015

Done. Anything else I need to fix?

@alexcrichton
Copy link
Member

@bors: r+ 39aa668

Thanks!

Manishearth added a commit to Manishearth/rust that referenced this pull request Apr 1, 2015
While trying to implement parallel ECS processing, I stumbled upon the need to mutate `Arc` contents. The only existed method that allowed that was `make_unique`, but it has issues:
  - it may clone the data as if nothing happened, where the program may just need to crash
  - it forces `Clone` bound, which I don't have

The new `try_unique` allows accessing the contents mutably without `Clone` bound and error out if the pointer is not unique.
@bors
Copy link
Contributor

bors commented Apr 1, 2015

⌛ Testing commit 39aa668 with merge 6a7d49e...

@bors
Copy link
Contributor

bors commented Apr 1, 2015

💔 Test failed - auto-mac-32-opt

@alexcrichton alexcrichton merged commit 39aa668 into rust-lang:master Apr 2, 2015
@kvark
Copy link
Contributor Author

kvark commented Apr 2, 2015

@alexcrichton but.. bors showed that my docs were incorrect. Are you sure about merging?

@alexcrichton
Copy link
Member

No worries! I fixed them in the rollup

@kvark
Copy link
Contributor Author

kvark commented Apr 2, 2015

@alexcrichton thanks! My machine takes ages to build and check everything...

@kvark kvark deleted the try_unique branch April 2, 2015 01:47
@lilyball
Copy link
Contributor

lilyball commented Apr 3, 2015

Is there some reason this was named unique() when the corresponding std::rc function is called get_mut?

@kvark
Copy link
Contributor Author

kvark commented Apr 3, 2015

@kballard no, I wasn't aware of get_mut, so the unique name was a mistake (unless we want rc::unique()).

@lilyball
Copy link
Contributor

lilyball commented Apr 3, 2015

There's a lot of precedent in the collection types (including slices) for a get_mut that returns an Option<&mut T>, which I believe is why I called the rc function that. And more generally, I think it's a better name.

@kvark
Copy link
Contributor Author

kvark commented Apr 3, 2015

@kballard I'll make a PR tonight to change it.

@lilyball
Copy link
Contributor

lilyball commented Apr 3, 2015

Awesome, thanks. That's one less thing for me to try and remember (since I'm on vacation right now 😀)

bors added a commit that referenced this pull request Apr 5, 2015
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.

6 participants