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

Implement new method HashMap.insert_or_update_with() #6815

Closed
wants to merge 2 commits into
base: incoming
from

Conversation

Projects
None yet
4 participants
@kballard
Contributor

kballard commented May 29, 2013

std::hashmap::HashMap.insert_or_update_with() is basically the opposite
of find_or_insert_with(). It inserts a given key-value pair if the key
does not already exist, or replaces the existing value with the output
of the passed function if it does.

This is useful because replicating this with existing functionality is awkward, especially with the current borrow-checker. In my own project I have code that looks like

if match map.find_mut(&key) {
    None => { true }
    Some(x) => { *x += 1; false }
} {
    map.insert(key, 0);
}

and it took several iterations to make it look this good. The new function turns this into

map.insert_or_update_with(key, 0, |_,x| *x += 1);
@erickt

This comment has been minimized.

Show comment
Hide comment
@erickt

erickt May 30, 2013

Contributor

Another implementation of this could be:

map.insert(key, map.pop(&key).map_default(0, |x| *x + 1))

But your way is much more elegant.

Contributor

erickt commented May 30, 2013

Another implementation of this could be:

map.insert(key, map.pop(&key).map_default(0, |x| *x + 1))

But your way is much more elegant.

@erickt

This comment has been minimized.

Show comment
Hide comment
@erickt

erickt May 30, 2013

Contributor

That said, why do you return the value instead of a bool like HashMap.insert? Is there a precedent to that style? If so, does that precedent also hold for HashMap.insert? Or if there is none, should insert_or_modify_with return bool instead?

Other than this question, this request looks good to me.

Contributor

erickt commented May 30, 2013

That said, why do you return the value instead of a bool like HashMap.insert? Is there a precedent to that style? If so, does that precedent also hold for HashMap.insert? Or if there is none, should insert_or_modify_with return bool instead?

Other than this question, this request looks good to me.

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard May 30, 2013

Contributor

I actually don't care about the return value at all, but I returned it for parity with find_or_insert_with(). Returning the value seems slightly more useful than returning bool; if I want a bool value I can always modify a captured variable in the passed lambda.

Contributor

kballard commented May 30, 2013

I actually don't care about the return value at all, but I returned it for parity with find_or_insert_with(). Returning the value seems slightly more useful than returning bool; if I want a bool value I can always modify a captured variable in the passed lambda.

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard May 30, 2013

Contributor

Of course, find_or_insert() is a "find" function, where returning the value is part of its core functionality. insert_or_modify_with() isn't, so the return value choice here is much less obvious. The more I think about it, the more a bool return value actually seems sensible, for parity with insert().

Contributor

kballard commented May 30, 2013

Of course, find_or_insert() is a "find" function, where returning the value is part of its core functionality. insert_or_modify_with() isn't, so the return value choice here is much less obvious. The more I think about it, the more a bool return value actually seems sensible, for parity with insert().

@erickt

This comment has been minimized.

Show comment
Hide comment
@erickt

erickt May 30, 2013

Contributor

@kballard and I talked things through on IRC, but we didn't come up with a good conclusion. Haskell's Data.Map has a couple similar functions, namely insertWith and insertLookupWithKey. A slight variation on this function could cover all the use cases of Haskell's functions:

fn insert_with(&mut self, key: K, value: V, f: &fn(&V, &V) -> V) -> Option<V>
fn insert_with_key(&mut self, key: K, value: V, f: &fn(&K, &V, &V) -> V) -> Option<V>

where the first argument of the insert_with closure is the old value, the second is the new value, and the return value is the old value if existed. insert_with_key also includes the key if necessary. The downside to this is that it makes @kballard's example program:

map.insert_with(key, 0, |x, _| *x + 1);

Which is a little uglier. I don't think this is that bad. What does everyone else think?

Contributor

erickt commented May 30, 2013

@kballard and I talked things through on IRC, but we didn't come up with a good conclusion. Haskell's Data.Map has a couple similar functions, namely insertWith and insertLookupWithKey. A slight variation on this function could cover all the use cases of Haskell's functions:

fn insert_with(&mut self, key: K, value: V, f: &fn(&V, &V) -> V) -> Option<V>
fn insert_with_key(&mut self, key: K, value: V, f: &fn(&K, &V, &V) -> V) -> Option<V>

where the first argument of the insert_with closure is the old value, the second is the new value, and the return value is the old value if existed. insert_with_key also includes the key if necessary. The downside to this is that it makes @kballard's example program:

map.insert_with(key, 0, |x, _| *x + 1);

Which is a little uglier. I don't think this is that bad. What does everyone else think?

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard May 30, 2013

Contributor

After thinking about this some more, one worry I have with the new functions is it won't let me modify the existing value, instead I have to create a brand new one. So I can't say something like

map.insert_with(key, ~[5], |x, _| { x.push(5); x })
Contributor

kballard commented May 30, 2013

After thinking about this some more, one worry I have with the new functions is it won't let me modify the existing value, instead I have to create a brand new one. So I can't say something like

map.insert_with(key, ~[5], |x, _| { x.push(5); x })
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 30, 2013

Contributor

I think the most general pattern is this:

fn mangle(
  &mut self,
  k: K,
  not_found: &fn(&K) -> V,
  found: &fn(&K, &mut V)) -> bool

and we should add that, and then we can build find_or_insert and find_or_insert_with on top of it.

Contributor

nikomatsakis commented May 30, 2013

I think the most general pattern is this:

fn mangle(
  &mut self,
  k: K,
  not_found: &fn(&K) -> V,
  found: &fn(&K, &mut V)) -> bool

and we should add that, and then we can build find_or_insert and find_or_insert_with on top of it.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 30, 2013

Contributor

This code, incidentally, becomes:

map.mangle(key, |_| 0, |_, x| *x += 1)
Contributor

nikomatsakis commented May 30, 2013

This code, incidentally, becomes:

map.mangle(key, |_| 0, |_, x| *x += 1)
@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 30, 2013

Contributor

I guess I have no strong opinion about what mangle should return, perhaps &mut V would be best, since this function is all about maximum flexibility.

Contributor

nikomatsakis commented May 30, 2013

I guess I have no strong opinion about what mangle should return, perhaps &mut V would be best, since this function is all about maximum flexibility.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 30, 2013

Contributor

and the proper name is probably insert_or_update

Contributor

nikomatsakis commented May 30, 2013

and the proper name is probably insert_or_update

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard Jun 1, 2013

Contributor

@nikomatsakis: Building find_or_insert on top of mangle is actually non-trivial, since it needs to return the found/inserted value. I don't see any easy way to do that without performing a second find after the mangle, which is rather non-optimal.

Two alternatives I can see are either to have mangle return the bucket index, which would require it to be a private function (did you intend for it to be private or public?), or to have it return &'a V.

Contributor

kballard commented Jun 1, 2013

@nikomatsakis: Building find_or_insert on top of mangle is actually non-trivial, since it needs to return the found/inserted value. I don't see any easy way to do that without performing a second find after the mangle, which is rather non-optimal.

Two alternatives I can see are either to have mangle return the bucket index, which would require it to be a private function (did you intend for it to be private or public?), or to have it return &'a V.

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard Jun 1, 2013

Contributor

Also, presumably it should be insert_or_update_with, since the other functions that take a closure have the _with suffix.

Contributor

kballard commented Jun 1, 2013

Also, presumably it should be insert_or_update_with, since the other functions that take a closure have the _with suffix.

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard Jun 1, 2013

Contributor

AFAIK I can't actually build find_or_insert and friends on top of mangle without working once functions.

self.mangle(k, |_| v, |_,_| ())

complains (rightly) that |_| v is trying to move a value from its environment.

Contributor

kballard commented Jun 1, 2013

AFAIK I can't actually build find_or_insert and friends on top of mangle without working once functions.

self.mangle(k, |_| v, |_,_| ())

complains (rightly) that |_| v is trying to move a value from its environment.

@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard Jun 1, 2013

Contributor

Looks like I can do the job using Option and swap_unwrap, but I assume this is now producing suboptimal code.

Contributor

kballard commented Jun 1, 2013

Looks like I can do the job using Option and swap_unwrap, but I assume this is now producing suboptimal code.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Jun 1, 2013

Contributor

A couple of comments:

  1. Make mangle return &mut V, not a hashtable index. It's supposed to be something an outside user would use so it should not expose an internal implementation detail, I don't think.
  2. While you're modifying this stuff, make find_or_insert_with return &mut V, not &V (issue #6394).
  3. This is a good example where once fns would be useful, which is nice. For the time being, you can address the inefficiency by modifying the signature of mangle to take an extra "user-supplied argument" as follows:
fn mangle<A>(
  &mut self,
  k: K,
  a: A,
  not_found: &fn(&K, A) -> V,
  found: &fn(&K, &mut V, A)) -> bool

in which case you can write find_or_insert as follows:

self.mangle(k, v, |_, v| v, |_,_,_| ())
Contributor

nikomatsakis commented Jun 1, 2013

A couple of comments:

  1. Make mangle return &mut V, not a hashtable index. It's supposed to be something an outside user would use so it should not expose an internal implementation detail, I don't think.
  2. While you're modifying this stuff, make find_or_insert_with return &mut V, not &V (issue #6394).
  3. This is a good example where once fns would be useful, which is nice. For the time being, you can address the inefficiency by modifying the signature of mangle to take an extra "user-supplied argument" as follows:
fn mangle<A>(
  &mut self,
  k: K,
  a: A,
  not_found: &fn(&K, A) -> V,
  found: &fn(&K, &mut V, A)) -> bool

in which case you can write find_or_insert as follows:

self.mangle(k, v, |_, v| v, |_,_,_| ())
@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard Jun 1, 2013

Contributor

I wasn't sure if you intended for it to be public or private. I'm happy making it public.

Contributor

kballard commented Jun 1, 2013

I wasn't sure if you intended for it to be public or private. I'm happy making it public.

kballard added some commits Jun 1, 2013

Refactor some hashmap code into a new private function mangle()
Add new private hashmap function

    fn mangle(&mut self,
              k: K,
              not_found: &fn(&K) -> V,
              found: &fn(&K, &mut V)) -> uint

Rewrite find_or_insert() and find_or_insert_with() on top of mangle().

Also take the opportunity to change the return type of find_or_insert()
and find_or_insert_with() to &'a mut V. This fixes #6394.
Add new function hashmap.insert_or_update_with()
fn insert_or_update_with<'a>(&'a mut self,
                             k: K,
                             f: &fn(&K, &mut V)) -> &'a V
@kballard

This comment has been minimized.

Show comment
Hide comment
@kballard

kballard Jun 2, 2013

Contributor

The latest commits make the suggested changes.

Contributor

kballard commented Jun 2, 2013

The latest commits make the suggested changes.

@erickt

This comment has been minimized.

Show comment
Hide comment
@erickt

erickt commented on 75f1b7f Jun 2, 2013

r+

@erickt

This comment has been minimized.

Show comment
Hide comment
@erickt

erickt Jun 2, 2013

Contributor

@kballard: Awesome, thanks for doing this!

Contributor

erickt commented Jun 2, 2013

@kballard: Awesome, thanks for doing this!

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jun 2, 2013

Contributor

saw approval from erickt
at kballard@75f1b7f

Contributor

bors commented on 75f1b7f Jun 2, 2013

saw approval from erickt
at kballard@75f1b7f

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jun 2, 2013

Contributor

merging kballard/rust/hashmap-insert_or_modify_with = 75f1b7f into auto

Contributor

bors replied Jun 2, 2013

merging kballard/rust/hashmap-insert_or_modify_with = 75f1b7f into auto

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jun 2, 2013

Contributor

kballard/rust/hashmap-insert_or_modify_with = 75f1b7f merged ok, testing candidate = 14c3310

Contributor

bors replied Jun 2, 2013

kballard/rust/hashmap-insert_or_modify_with = 75f1b7f merged ok, testing candidate = 14c3310

This comment has been minimized.

Show comment
Hide comment
@bors

bors Jun 2, 2013

Contributor

fast-forwarding incoming to auto = 14c3310

Contributor

bors replied Jun 2, 2013

fast-forwarding incoming to auto = 14c3310

bors added a commit that referenced this pull request Jun 2, 2013

auto merge of #6815 : kballard/rust/hashmap-insert_or_modify_with, r=…
…erickt

`std::hashmap::HashMap.insert_or_update_with()` is basically the opposite
of `find_or_insert_with()`. It inserts a given key-value pair if the key
does not already exist, or replaces the existing value with the output
of the passed function if it does.

This is useful because replicating this with existing functionality is awkward, especially with the current borrow-checker. In my own project I have code that looks like

	if match map.find_mut(&key) {
		None => { true }
		Some(x) => { *x += 1; false }
	} {
		map.insert(key, 0);
	}

and it took several iterations to make it look this good. The new function turns this into

    map.insert_or_update_with(key, 0, |_,x| *x += 1);

@bors bors closed this Jun 2, 2013

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