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

Add Map::insert_with_key #3975

Closed
wants to merge 3 commits into from

Conversation

killerswan
Copy link
Contributor

As discussed in #2504, this adds an insert_with_key ([as in Haskell](http://hackage.haskell.org/packages/archive/containers/0.4.0.0/doc/html/Data-Map.html#g:5 as in Haskell)) to the Map trait.

@catamorphism
Copy link
Contributor

Thanks! Can you resubmit without the "let's see if it compiles" comment? :-)

@nikomatsakis
Copy link
Contributor

Can we give this another name? The name insert_with_key() makes no sense to me, every insert requires a key! Perhaps update() or insert_or_update()?

@nikomatsakis
Copy link
Contributor

Sorry, I shouldn't have said 'makes no sense', sounds kind of harsh. I just meant that the name did not give me a hint at what the function did or how it might differ from any other insert(). In any case, seems like a useful operation!

@killerswan
Copy link
Contributor Author

I'm about to push some fixes to this branch.

I've removed that comment about seeing if it works and stuck in a lower-level version which I expect to be faster. Now insert could be defined with insert_with_key, although I'm not sure we want to do that. Is rustc smart enough to optimize away something like |a,b,c| { c }?

I think I've clarified the tests a little bit, and I've also added an insert_with.

As far as names, how would update and update_with_key sound? I'm not that attached to the current names, but I've left them insert_with and insert_with_key like the Haskell versions.

This commit adds a lower-level implementation of the generic
`insert_with_key` which I expect to be faster. Now insert could be
defined with insert_with_key, too, although I'm not sure we want to do that.

This also clarifies the tests a bit and adds an `insert_with` function.
@killerswan
Copy link
Contributor Author

This push might still have a bug, but I have to go meet some people and don't have time to wait for tests to finish.

@killerswan
Copy link
Contributor Author

OK, passing time CFG_DISABLE_VALGRIND=1 make check. :D

@brson
Copy link
Contributor

brson commented Nov 25, 2012

Pushed as update and update_with_key. Thanks!

@brson brson closed this Nov 25, 2012
tmandry added a commit to tmandry/rust that referenced this pull request Jul 25, 2020
Changes:
- preparation for potential rustfmt 1.4.19 (rust-lang#4283)
- chore: backport 8157a3f0afe978d3e953420577f8344db7e905bf
- deps: bump rustc-ap to v669
- deps: bump rustc-ap-* to v668
- deps: bump rustc-ap* to v666
- Use correct span for match arms with the leading pipe and attributes (rust-lang#3975)
@tmandry tmandry mentioned this pull request Jul 25, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2020
Update rustfmt and rls

Closes rust-lang#74080, rust-lang#74081.

rls changes:
- deps: update racer and cargo

rustfmt changes:
- preparation for potential rustfmt 1.4.19 (rust-lang#4283)
- chore: backport 8157a3f0afe978d3e953420577f8344db7e905bf
- deps: bump rustc-ap to v669
- deps: bump rustc-ap-* to v668
- deps: bump rustc-ap* to v666
- Use correct span for match arms with the leading pipe and attributes (rust-lang#3975)
bors pushed a commit to rust-lang-ci/rust that referenced this pull request May 15, 2021
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.

4 participants