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

map::OccupiedEntryRef::key should return &K rather than &Q #421

Closed
programmerjake opened this issue Apr 17, 2023 · 1 comment · Fixed by #425
Closed

map::OccupiedEntryRef::key should return &K rather than &Q #421

programmerjake opened this issue Apr 17, 2023 · 1 comment · Fixed by #425

Comments

@programmerjake
Copy link
Member

returning Q unnecessarily throws away the key type, which may have extra information needed by the user, e.g. the Rc you want to clone.

example where returning K is useful:

struct Interner<T: Eq + Hash> {
    values: HashMap<Rc<T>, ()>,
}

impl<T: Eq + Hash> Interner<T>
where
    Rc<T>: for<'a> From<&'a T>,
{
    fn intern(&mut self, v: &T) -> Rc<T> {
        match self.values.entry_ref(v) {
            EntryRef::Occupied(entry) => Rc::clone(entry.key()), // getting `&Rc<T>` instead of `&T` avoids allocating a new `Rc<T>`
            EntryRef::Vacant(entry) => Rc::clone(entry.insert_entry(()).key())
        }
    }
}
@Amanieu
Copy link
Member

Amanieu commented Apr 21, 2023

I think this is a good change and would be happy to accept a PR for it (since EntryRef is still unstable).

bors added a commit that referenced this issue Apr 21, 2023
Change key to return `&K` rather than `&Q`

also changes `EntryRef::and_replace_entry_with` and `OccupiedEntryRef::replace_entry_with` to also give out `&K`

Fixes: #421
@bors bors closed this as completed in ecda296 Apr 21, 2023
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 a pull request may close this issue.

2 participants