search

JesperAxelsson / rust-intmap

Specialized hashmap for u64 keys
MIT License
30 stars 10 forks source link

IntMap::insert has unexpected behaviour #26

Closed jakoschiko closed 2 years ago

jakoschiko commented 2 years ago

Currently, the IntMap::insert function doesn't modify the map if it already contains the key.

For me, this is confusing because:

Would it be reasonable to change the current behaviour of IntMap::insert? Or would it be better to add another function IntMap::insert_or_replace?

JesperAxelsson commented 2 years ago

In my day job I work with C# so was probably inspired by Dictionary.add (which throws an exception on duplicate key).

Personally I find the behavior a bit too "clever", but you make a good point that it's better to mirror the HashMap api to be a better drop-in replacement.

This would require a major version bump though.

jakoschiko commented 2 years ago

If you really want to change the current behavior, it would be nice to check also the other functions whether they correlate with the functions from std lib. It's probably a good idea to have some tests that compare these implementations. I'll be glad to help.

JesperAxelsson commented 2 years ago

That's a good point. If you are willing to implement this I'm all for it.

aldanor commented 2 years ago

@JesperAxelsson Wondering if it would be possible to change IntMap::insert() signature to match that of the standard library and return Option<V>? (or adding a separate method... although matching the stdlib API is always nice because as you said above, it would make it more of a drop-in replacement)

In some cases, it's absolutely critical and is the sole reason you're using the hashmap at all. For example you (maybe) overwrite a value at a specified key and want to check by how much the sum of the total elements has just changed. This can only be done if the Option<V> (the old value if it has been replaced) is returned back to you.

Plus, as already noted by the OP above, this is very unexpected behaviour in general (as all other hashmap libraries generally try to mimic the stdlib) and makes inserting+replacing a very weird procedure here.