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.

Sign up for GitHub

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

Jump to bottom

Implement get_pair for HashMap #46992

Closed
wants to merge 2 commits into from
Closed

Implement get_pair for HashMap #46992

wants to merge 2 commits into from

Conversation

Diggsey
Copy link
Contributor

@Diggsey Diggsey commented Dec 25, 2017

Fixes #43143

@rust-highfive
Copy link
Collaborator

r? @Kimundi

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

@ghost
Copy link

ghost commented Dec 25, 2017

Any reason for not adding the same method to BTreeMap?

@Diggsey
Copy link
Contributor Author

Diggsey commented Dec 25, 2017

No, it just wasn't mentioned in the issue - I will add it there too.

@nvzqz
Copy link
Contributor

nvzqz commented Dec 25, 2017

Is it possible to have get_pair_mut that returns Option<(&K, &mut V)>?

@Diggsey
Copy link
Contributor Author

Diggsey commented Dec 25, 2017

@nvzqz The entry api covers that case

@nvzqz
Copy link
Contributor

nvzqz commented Dec 25, 2017

The entry api requires passing an owned key (K) whereas this allows passing any Q where K: Borrow<Q>.

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 27, 2017
@alexcrichton
Copy link
Member

ping @rust-lang/libs, thoughts on this API/naming?

@alexcrichton alexcrichton added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2018
@sfackler
Copy link
Member

sfackler commented Jan 9, 2018

#47259 added HashMap::remove_entry - the namings for the remove and access should hopefully match. get_entry may be confusing though in comparison to entry?

@RReverser
Copy link
Contributor

I'll back get_pair_mut suggestion, but even without it this addition looks pretty good.

@RReverser
Copy link
Contributor

RReverser commented Jan 18, 2018 •
edited
Loading

Although get_pair_mut could instead be occupied_entry(&mut self, &K) -> Option<OccupiedEntry> instead to provide all the convenient mutation APIs for free, so probably unrelated to this PR.

@carols10cents carols10cents added I-nominated I-needs-decision Issue: In need of a decision. labels Jan 22, 2018
@Diggsey
Copy link
Contributor Author

Diggsey commented Jan 31, 2018

I've added the BTreeMap method too. I'm not going to add the mutable versions as part of this PR as I think there are still some questions about what the API should look like.

@aturon aturon assigned aturon and unassigned Kimundi Feb 6, 2018
@aturon
Copy link
Member

aturon commented Feb 7, 2018

Sorry for the delay on this from the libs team!

I think that get_entry is too confusing, as @sfackler mentioned, but get_pair is not very self-explanatory.

How about get_key_value?

@pietroalbini pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed I-needs-decision Issue: In need of a decision. I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Feb 19, 2018
@pietroalbini
Copy link
Member

@Diggsey someone from the libs team replied with a suggestion for another name, can you implement that?

@pietroalbini
Copy link
Member

@Diggsey thanks for taking the time to send this PR! Unfortunately, there was no activity for more than two weeks, so I'm closing it. If you have time to work on it again, feel free to open another PR with the new changes, we'll be happy to review and merge it!

@Diggsey Diggsey mentioned this pull request Mar 24, 2018
Merged
kennytm added a commit to kennytm/rust that referenced this pull request Mar 26, 2018
@kennytm
Rollup merge of rust-lang#49346 - Diggsey:hashmap-get-pair, r=Mark-Si…
ca521d3 
…mulacrum

Implement get_key_value for HashMap, BTreeMap

Fixes rust-lang#43143

Follow up from rust-lang#46992
TimNN added a commit to TimNN/rust that referenced this pull request Mar 26, 2018
@TimNN
Rollup merge of rust-lang#49346 - Diggsey:hashmap-get-pair, r=Mark-Si…
d601e74 
…mulacrum

Implement get_key_value for HashMap, BTreeMap

Fixes rust-lang#43143

Follow up from rust-lang#46992
@Diggsey Diggsey mentioned this pull request Apr 28, 2018
Closed
RReverser added a commit to cloudflare/wirefilter that referenced this pull request Feb 20, 2019
@RReverser
Use ordermap to get (&K, &V) pairs
3320448 
We won't need this when rust-lang/rust#46992 is implemented, but for now this is the easiest (and safe) way to untie Filter AST from its string representation, so it can live as long as the context does.
RReverser added a commit to cloudflare/wirefilter that referenced this pull request Feb 20, 2019
@RReverser
Use ordermap to get (&K, &V) pairs
0f0c9a7 
We won't need this when rust-lang/rust#46992 is implemented, but for now this is the easiest (and safe) way to untie Filter AST from its string representation, so it can live as long as the context does.
RReverser added a commit to cloudflare/wirefilter that referenced this pull request Feb 20, 2019
@RReverser
Use ordermap to get (&K, &V) pairs
347955a 
We won't need this when rust-lang/rust#46992 is implemented, but for now this is the easiest (and safe) way to untie Filter AST from its string representation, so it can live as long as the context does.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aturon aturon

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.