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

Allow a `HashMap` and `BTreeMap` entry to be replaced. #44286

Open
Binero opened this Issue Sep 3, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@Binero
Contributor

Binero commented Sep 3, 2017

At the moment the only way to replace a map entry with a new one, swallows the old key. In cases the user wants to keep the old key, the only way to do so is to first remove_entry and to then insert a new one. This requires two map look-ups.

As the Entry API aims to make operations that would otherwise require two look-ups into operations that require only one, the replacing of an entry is an obvious hole in the API.

@Binero Binero changed the title from Allow to replace a `HashMap` and `BTreeMap` entry to be replaced. to Allow a `HashMap` and `BTreeMap` entry to be replaced. Sep 3, 2017

@stepancheg

This comment has been minimized.

Contributor

stepancheg commented Oct 6, 2017

While it is indeed a missing API, there are several issues with this new API implemented in PR #44278:

  • it seems to panics if take_key was called before (it is not public API)
  • it replaces both key and value, so
    • is not possible to replace only key or only value (there's insert function which replaces only value)
    • it does do completely independent things in one operation
  • it is not possible to replace key in VacantEntry (it is pointless)
  • it takes self, while it could take &mut self, so replace could be called again if it is needed for some reason
  • key replacement it is against least surprise principle

I think HashMap::replace should be different:

impl OccupiedEntry<...> {
    fn replace_value(&mut self, new_value: V) -> V {
        replace value with new value, do not touch the key.
    }
}

And probably that's it.

If we need to replace key for some reason, replace_key operation needs to be added to both OccupiedEntry and VacantEntry.

@Binero

This comment has been minimized.

Contributor

Binero commented Oct 6, 2017

  • take_key cannot be called before, all functions that call it consume the entry.
  • The point is to be able to swap out the key without multiple lookups. replace_value beats the point.
  • A VacantEntry does not have a key, so it's unclear how the function would work.
@stepancheg

This comment has been minimized.

Contributor

stepancheg commented Oct 6, 2017

take_key cannot be called before, all functions that call it consume the entry.

Oh, it is not public API, thanks.

The point is to be able to swap out the key without multiple lookups. replace_value beats the point.

But this function does not allow to replace key without replacing value.

BTW, there's already a function to replace value, it's called insert.

So if we had replace_key instead of current replace, then current replace could be emulated as:

occupied_entry.insert(new_value);
occupied_entry.replace_key()

replace_key is not possible now, and replace is redundant if we had replace_key.

A VacantEntry does not have a key, so it's unclear how the function would work.

Please ignore that.

@Binero

This comment has been minimized.

Contributor

Binero commented Oct 9, 2017

I will take a look at replace_key when I have time, probably next week.

@Binero

This comment has been minimized.

Contributor

Binero commented Oct 9, 2017

@stepancheg I think it's probably better to have replace_key consume the entry, because the semantics of the function would otherwise change after the first call.

Binero added a commit to Binero/rust that referenced this issue Oct 9, 2017

Addressed issues raised in rust-lang#44286.
This commit renames the `replace` function to `replace_entry`, and
creates a seperate `replace_key` function for `OccupiedEntry`. The
original `replace` function did not solve the use-case where the
key needed to be replaced, but not the value. Documentation and
naming has also been updated to better reflect what the original
replace function does.

Binero added a commit to Binero/rust that referenced this issue Nov 11, 2017

Addressed issues raised in rust-lang#44286.
This commit renames the `replace` function to `replace_entry`, and
creates a seperate `replace_key` function for `OccupiedEntry`. The
original `replace` function did not solve the use-case where the
key needed to be replaced, but not the value. Documentation and
naming has also been updated to better reflect what the original
replace function does.

bors added a commit that referenced this issue Nov 11, 2017

Auto merge of #45152 - Binero:master, r=dtolnay
Addressed issues raised in #44286. (`OccupiedEntry::replace_entry`)

This commit renames the `replace` function to `replace_entry`, and
creates a seperate `replace_key` function for `OccupiedEntry`. The
original `replace` function did not solve the use-case where the
key needed to be replaced, but not the value. Documentation and
naming has also been updated to better reflect what the original
replace function does.

bors added a commit that referenced this issue Nov 11, 2017

Auto merge of #45152 - Binero:master, r=dtolnay
Addressed issues raised in #44286. (`OccupiedEntry::replace_entry`)

This commit renames the `replace` function to `replace_entry`, and
creates a seperate `replace_key` function for `OccupiedEntry`. The
original `replace` function did not solve the use-case where the
key needed to be replaced, but not the value. Documentation and
naming has also been updated to better reflect what the original
replace function does.
@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Mar 17, 2018

This has been in Nightly for long enough that I’m inclined to stabilize, but I don’t quite understand the use case. @Binero, can you explain some more why it can be important to recover the old key rather than the new one (passed to HashMap::entry() which hashes and compares equal?

@Binero

This comment has been minimized.

Contributor

Binero commented Mar 17, 2018

@SimonSapin

It is quite a niche function, so it's certainly not instantly obvious where this would be useful.

That said, it has some uses when you want to for example lower the memory footprint of a HashMap by swapping out an Rc<T> with another, equivalent, Rc<T> that you are already using elsewhere.

If either T or the HashMap is large enough, this could reduce the memory footprint of the application by quite a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment