-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Tracking Issue for map_try_insert #82766
Comments
Is there a reason why this isn't implemented for |
@alecmocatta The sets have a different kind of |
Personally I feel the motivation still stands. let x = map.insert(0, ());
assert!(x.is_none());
let x = set.insert(0);
assert!(x); vs map.try_insert(0, ()).unwrap();
set.try_insert(0).unwrap(); The latter communicate "panic on duplicate" much more obviously IMO. Plus |
I think it is important that this API returns the |
Seems like this could benefit from a lazy |
Is the name of the method still open for debate? I found myself confusing it with the try methods of for example This is what I expected the method to look like; pub fn try_insert(&mut self, key: K, value: V) -> Result<Option<V>, TryReserveError>; Maybe it would be better to rename it to something like |
@Luro02 . I agree. If we ever add method, which will report both "already exists" and "allocation error" errors, it will be called |
From my experience, any |
I think The downside of this is it's rather long. Since even My proposition is |
I do think we need some way to handle the out-of-memory case. We don't have to solve that with this method, but I think we should think about naming with that in mind. One naming possibility: |
Maybe |
I think it's a terrible idea:
I think such name would be error prone. |
I offer the name |
I propose we take the same naming. Essentially they do the same thing, which is: get the thing already inside or insert a new one, except the naming for this is clearer. It also help to have consisted naming across different types. I'm also not convinced that returning an let mut occupied = false;
let val : &mut Value = my_map.get_or_insert_with(key, ||{ occupied = true; Value::new() }); |
You can already do that with |
In my opinion there are several issues that need to be addressed:
// returns Some(value) if KV-pair already exists
pub fn insert_vacant(&mut self, key: K, value: V) -> Option<V>;
I do understand that in some situations it would be very useful to get a reference to the inserted value. In fact this need is exactly what brought me here. That being said, I think it should most definitely be a separate function with a more descriptive name. I have put together a RFC for this which you can find here. |
Suggestion: returned error might contain the owned key as well as the value. |
That something we could add in https://doc.rust-lang.org/std/collections/hash_map/struct.OccupiedError.html |
I find that I often need something like this, but it isn't a great fit. The pattern I see in a few places is maybe fundamentally different to this, but maybe more like. impl<K: Eq + Hash, V, S> HashMap<K, V, S> {
pub fn get_or<Q: ?Sized>(&mut self, k: &Q, v: V) -> Option<&V>where K: Borrow<Q>, Q: Hash + Eq { ... }
pub fn get_mut_or<Q: ?Sized>(&mut self, k: &Q, v: V) -> Option<&mut V>where K: Borrow<Q>, Q: Hash + Eq {
if let Some(v) = self.get(k) {
v
} else {
self.entry(k.to_owned()).or_insert(v)
}
}
pub fn get_or_with<Q: ?Sized, F: FnOnce() -> V>(&mut self, k: &Q, f: F) -> Option<&V>where K: Borrow<Q>, Q: Hash + Eq { ... }
pub fn get_mut_or_with<Q: ?Sized, F: FnOnce() -> V>(&mut self, k: &Q, f: F) -> Option<&mut V>where K: Borrow<Q>, Q: Hash + Eq { ... }
impl<K: Eq + Hash, V: Default, S> HashMap<K, V, S> {
pub fn get_or_default<Q: ?Sized>(&mut self, k: &Q) -> Option<&V>where K: Borrow<Q>, Q: Hash + Eq { ... }
pub fn get_mut_or_default<Q: ?Sized>(&mut self, k: &Q) -> Option<&mut V>where K: Borrow<Q>, Q: Hash + Eq { ... }
} That is, I want to |
What is the current status of this feature? |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment has been minimized.
This comment has been minimized.
Personally, I'd love to see something like this that takes a function both for the key and the value
Since currently, there isn't a way to do "get_or_insert" without looking the key up twice or pre-cloning the key. |
Usually the naming for such method would be |
Yeah naming it Then |
I like this functionality, I wouldn't mind a name change, but nothing else is required. However, I would personally perfer OccupiedError be changed from:
To:
I can see both are useful -- depends on if someone wants the entry, to maybe do something else with, or they want the key they passed in. One big advantage of this change it it removes the lifetime. With the current OccupiedError, I find it hard to use |
why not return both the key/value you tried to insert and the |
In my experience fn get_or_insert<T>(&mut self, default: T) -> &T
if map.contains_key(&key) {
Err(KeyExistsError)
} else {
let old_value = map.insert(key, new_value);
assert!(old_value.is_none())
Ok(())
} With |
Pardon my beginner question, but why the proposed As I understand it's supposed to be analogous to This doesn't change much as |
The thread seem to confuse the purpose of this function, that why a renaming is more than vital, it seems get_or_insert would be a terrible idea. to be clear the goal is to be analog to: use std::collections::hash_map::{Entry, HashMap};
fn main() -> Result<(), ()> {
let mut foo = HashMap::new();
// try_insert
let _v = match foo.entry("toto") {
Entry::Occupied(_) => return Err(()),
Entry::Vacant(v) => Ok(v.insert(())),
}?;
Ok(())
} Someone can use entry like that to avoid double lookup. @Quba1 it's always a good practice to return the thing the user created in a collection. The user can then ignore it or not. You push an item on a vector but want to use it afterward you need to do a |
How about Maybe it's a good idea to make a poll to move the naming issue forward? @Stargateur that makes a lot of sense. thanks for explaining |
This comment was marked as spam.
This comment was marked as spam.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Today I went to use this with Option but then I looked it up and saw that it's actually a method on HashMap. |
This function adds an entry, erroring out if the new key conflicts with an existing one... That's called The current With this I'm not saying the current (*) SQL calls this same operation "merge" (or sometimes "upsert"), but I'd say those names make little sense in a hashmap with only two "columns". |
This comment has been minimized.
This comment has been minimized.
Another reason to reconsider the name |
Feature gate:
#![feature(map_try_insert)]
This is a tracking issue for
BTreeMap::try_insert
andHashMap::try_insert
.Unlike
.insert()
,.try_insert()
does not overwrite existing values, and has a meaningful error type with more context. See #82764Public API
Steps / History
Unresolved Questions
OccupiedError
implement Error (and Display)? Add {BTreeMap,HashMap}::try_insert #82764 (comment)OccupiedError
have public fields, or accessors? Add {BTreeMap,HashMap}::try_insert #82764 (comment)try_
might imply it returns anErr
when allocating fails, which is not the case. Maybe rename it toinsert_new
or something?key: K
argument into theOccupiedError
?The text was updated successfully, but these errors were encountered: