-
Notifications
You must be signed in to change notification settings - Fork 49
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
Implemented HashMap::try_insert
#74
Conversation
HashMap::try_insert
.HashMap::try_insert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good! Especially the effort you put into the documentation 😊
I've left some comments, mainly I think we should do the change to a Result
return value. This None
on success thing still confuses me 😅
…nor documentation issues
Ah, I saw just now that the existing |
Exactly! Gonna work on this. |
#76 has now landed! |
All done I think. |
I still think we should use the |
See #77 for a way to recover the value. |
I'm not quite sold on this proposal. Although it does provide some useful functionality which is not provided in our current public API, I seems a bit cumbersome and unergonomic. Also, since this is a public API, we'd probably have to implement the |
Can/did you run Regarding the fields, I don't actually know; can you destructure a struct if it's fields are private? I would like the user to be able to do match map.try_insert(&i, value, &guard) {
Ok(new) => {...},
Err(TryInsertError { old, not_inserted } ) => {...}
} |
Nope, you can't. I think you can deconstruct them inside the |
Just did it:
Dunno why |
Then I'm against making them private. I think it's not worth for them to be private if we lose the destructuring |
Makes sense. @jonhoo what do you think? |
I am strongly in favor of both fields being public. We could mark it |
Alright, public it is. Marking things as I guess the only things left to do are:
|
The biggest downside of |
Fair enough. Seems unlikely indeed. |
If an entry already exists in the map for a given key, the existing `HashMap::insert` replaces it. That is not always what you want. This patch adds `HashMap::try_insert`, which returns an error if the key is already present in the map.
Thanks @jonhoo and @domenicquirl for the help! 😁️ |
Thanks for taking the time to implement it, and to follow up on all our nitpicking 😅 |
Follow up on #58.
This change is