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

Implement HashMap.find_with_or_insert_with() #14196

Merged
merged 5 commits into from May 16, 2014

Conversation

chris-morgan
Copy link
Member

This used to be called mangle and was removed when the Robin Hood hash map came along, but it is a useful thing to have in certain situations (I just hit it with my Teepee header representation), so I want it back.

The method is renamed to find_with_or_insert_with, also with the parameters swapped to make sense—find and then insert, not insert and then find.

/cc @cgaebel

This was removed when the Robin Hood hash map came along, but it is a
useful thing to have.

The comment is taken directly from what was there before (e.g. in 0.9)
but with appropriate language changes (like `StrBuf` instead of `~str`).
@chris-morgan
Copy link
Member Author

See https://github.com/teepee/teepee/blob/master/src/httpcommon/headers/mod.rs#L307..L314 and https://github.com/teepee/teepee/blob/master/src/httpcommon/headers/mod.rs#L347..L354 for the case where I want it.

(Removed from PR description text when it became out of date:

I’m not terribly enthusiastic about the name, but there’s historical precedent for mangle, so let it be that for now. find_with_or_insert_with is what occurs to me as a replacement in keeping with what else is there, but it is a bit of a mouthful. I would still be in favour of renaming it to find_with_or_insert_with over mangle.

)

@emberian
Copy link
Member

It seems that this is a legitimate usecase of a mangle.

@cgaebel
Copy link
Contributor

cgaebel commented May 14, 2014

This looks good to me, but I really like the name find_with_or_insert_with over mangle. It's a lot more descriptive.

I still think that there's some space for a better HashMap API here. Since the only efficiency gain is in not allowing the same key to be hashed twice, maybe there could be a function like HashMap::hash_key<'a>(k: &'a K) -> HashedKey<'a>, and then add versions of each hashmap operation that works on HashedKeys instead of keys.

Just my little design ramblings. Feel free to merge as is.

/// }
///
/// for (k, v) in map.iter() {
/// println!("{} -> {:?}", *k, *v);
Copy link
Member

Choose a reason for hiding this comment

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

We've been generally avoiding :?, does {} not suffice here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just the example that existed in 0.9 with the fixes that were necessary to get it to compile. Seeing as there appears to be acceptance of reintroducing this method, I’ll improve the example now.

This also entails swapping the order of the find and insert callbacks so
that their order matches the order of the terms in the method name.
@chris-morgan chris-morgan changed the title Reinstate HashMap.mangle(). Implement HashMap.find_with_or_insert_with() May 15, 2014
@chris-morgan
Copy link
Member Author

I’ve fixed up the examples to skip using StrBuf altogether and shifted away from the name mangle (including changing the order of found and not_found to match the name).

r?

///
/// for (k, v) in map.iter() {
/// println!("{} -> {}", *k, *v);
/// }
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be a little clearer if you asserted equality instead of printing, because I can't see what it prints without actually running the code locally. Instead you could use something like

assert_eq!(map.len(), 3);
assert_eq!(map.get("a key"), vec!["value", "new value"]);
asesrt_eq!(map.get("b key"), vec!["value", "new value"]);
assert_eq!(map.get("z key"), vec!["new value", "value"]);

This lets us test it automatically and also makes it more obvious what
the expected result is.
@chris-morgan
Copy link
Member Author

OK, not quite so much my fault; caused by #14240.

bors added a commit that referenced this pull request May 16, 2014
This used to be called `mangle` and was removed when the Robin Hood hash map came along, but it is a useful thing to have in certain situations (I just hit it with my Teepee header representation), so I want it back.

The method is renamed to `find_with_or_insert_with`, also with the parameters swapped to make sense—find and then insert, not insert and then find.

/cc @cgaebel
@bors bors closed this May 16, 2014
@bors bors merged commit ff98afe into rust-lang:master May 16, 2014
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 this pull request may close these issues.

None yet

7 participants