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

Add member operations to collections #26531

Closed
wants to merge 5 commits into from
Closed

Add member operations to collections #26531

wants to merge 5 commits into from

Conversation

seppo0010
Copy link
Contributor

Set and Map will may the type on get, insert and remove using
get_member, insert_member and remove_member.

Previous to this commit, there is no way to recovering the key from a
map upon removal, and it might even be useful for get because two
elements that are equal are not necessarily the same, or
indistinguishible.

This patch fixes rust-lang/rfcs#691 and helps with rust-lang/rfcs#690

Set and Map will may the type on get, insert and remove using
get_member, insert_member and remove_member.

Previous to this commit, there is no way to recovering the key from a
map upon removal, and it might even be useful for get because two
elements that are equal are not necessarily the same, or
indistinguishible.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gankro (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@seppo0010
Copy link
Contributor Author

I don't like the code duplication. It can probably be fixed by making the old methods call their _member alternative and .map(|x| x.1), but since the new methods are unstable I'm not sure if I can do that without breaking the universe.

I'm not in love of the member word. I want something similar to entry. I do like to have the methods returning both the key and the value (and not choosing one or the other), and ideally the set and the map would be named the same.

/// ```
#[unstable(feature = "collection_member",
reason="member stuff is unclear")]
pub fn get_member(&mut self, value: &T) -> Option<&T> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For sets I'd argue this can just be get, but I understand symmetry is a concern.

@Gankra
Copy link
Contributor

Gankra commented Jun 23, 2015

You can implement stable methods in terms of unstable methods with no negative affects. Implementation details are Implementation details.

@Gankra
Copy link
Contributor

Gankra commented Jun 23, 2015

CC @rust-lang/libs @apasel422

This will likely require a full RFC to accept, but I'm ok with hashing out basic details or discussion here for now.

@apasel422
Copy link
Contributor

This functionality would be great to have, but I agree that the naming isn't ideal. It's too bad we can't change the existing methods' return types due to backwards-compatibility concerns.

In my own code, I've been using

fn get<Q: ?Sized>(&self, key: &Q) -> Option<&V>;
fn find<Q: ?Sized>(&self, key: &Q) -> Option<(&K, &V)>;

but it's unclear how that could be adapted to {insert, remove}. All I can come up with for now is keyed_{insert, get, get_mut, remove}.

Additionally, if these methods are added, I would like to see OccupiedEntry get an fn key(&self) -> &K method and an analogous fn keyed_remove(self) -> (K, V) method.

@seppo0010
Copy link
Contributor Author

I did not do get_mut because I don't think returning a mutable key would be a good idea, since the position in the map/set needs to be changed when the key is modified. Is it possible to do that? Or would be useful to have get_mut returning (&K, &mut V)?

@apasel422
Copy link
Contributor

I was implying

fn get_mut<Q: ?Sized>(&self, key: &Q) -> Option<(&K, &mut V)>;

@seppo0010
Copy link
Contributor Author

👍

I also like the keyed_ prefix better.

@seppo0010
Copy link
Contributor Author

Ugh, keyed has the problem that it does not make sense for a Set though.

@apasel422
Copy link
Contributor

I don't think it's necessary for maps and sets to have the same naming here. It'd be nice, but I don't think we need to shoehorn a less-than-ideal name into either collection type just for consistency's sake. That said, there may be something better than keyed that works for both:

// map methods (could be confused with the `Entry` API)
fn get_entry<Q: ?Sized>(&self, key: &Q) -> Option<(&K, &V)>;
fn get_entry_mut<Q: ?Sized>(&mut self, key: &Q) -> Option<(&K, &mut V)>;
fn remove_entry<Q: ?Sized>(&mut self, key: &Q) -> Option<(K, V)>;
fn insert_entry(&mut self, key: K, value: V) -> Option<(K, V)>;

// set methods
fn get_item<Q: ?Sized>(&self, item: &Q) -> Option<&T>;
fn remove_item<Q: ?Sized>(&mut self, item: &Q) -> Option<T>;
fn insert_item(&mut self, item: T) -> Option<T>;

For sets, use item suffix instead.

Also add keyed_get_mut.
@eddyb
Copy link
Member

eddyb commented Jun 24, 2015

👍 I want to use this in the compiler.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 24, 2015
@alexcrichton
Copy link
Member

I agree with @gankro that we likely want an RFC before we land these changes, but I can certainly see where these methods are useful (although I would perhaps want to bikeshed the name a bit)

@alexcrichton
Copy link
Member

Closing in favor of the associated RFC: rust-lang/rfcs#1175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HashSet should have a get function
6 participants