Adds a default-implemented SecondaryMap::remove

[aborgna-q]

No description provided.

[acl-cqc]

Hmmmm. I see what you are aiming at here, but thoughts:

1. The must_use-ness really belongs to the type of values, not the map: it may not make sense for all values, and should we really complicate the interface to the map in this way to support it?
2. IIUC, no warning is generated for remove, right? So remove shows how simple it is to bypass the warning. Folk could just do that themselves....
3. Maybe we should use Rust's HashMap as a guide what to do here. It has only one method, remove, returning an Option<V>. So we use default() rather than option, but that suggests to me that we should rename take to remove and leave it at that....??

[aborgna-q]

• must_use is a rust flag for methods where not checking the returned value is suspicious (e.g. methods that return a Result). This has nothing to do with relevant types, one can always explicitly ignore the results with let _ = method().

• Yes, I'm not sure about it either. take is a standard name for "retrieve and leave a default", so I wouldn't rename. But there are instances where we want to just drop the value, and remove would be similar with the rest of the API;

graph.remove_node(node);
hierarchy.remove(node);
secondary_map.???(node);

[acl-cqc]

• must_use is a rust flag for methods where not checking the returned value is suspicious (e.g. methods that return a Result). This has nothing to do with relevant types, one can always explicitly ignore the results with let _ = method().
• Yes, I'm not sure about it either. take is a standard name for "retrieve and leave a default", so I wouldn't rename. But there are instances where we want to just drop the value, and remove would be similar with the rest of the API;

graph.remove_node(node);
hierarchy.remove(node);
secondary_map.???(node);

Hmmmm, your second set of examples is quite convincing, I agree. So maybe HashMap is the odd one out (remove returning a value) ! Indeed, perhaps HashSet is a better example (in agreement with your 3) - here take is much as you say and remove returns a bool (true = mutation happened). How about that?

[acl-cqc]

I realize that's a bit harder to implement as remove is no longer strictly a function of take. Hmmm, so in fact there are two possibilities. Suppose we first do m.put(key, default()), then

1. m.remove(key) == true - in which case remove cannot be a function of take as it must inspect whether the entry was there - and explicitly storing the default is different from not storing anything, with this difference revealed only by remove. The italics sounds a bit nasty, so
2. m.remove(key) == false, which is to say, the original m.put(key, default()) was a no-op that didn't mutate the map. In this case the trait can define remove as take() != default(). This is neater in many ways but relies on a strong notion of equality.

I could go either way there!!

[aborgna-q]

Yes, that's the main problem with that.
SecondaryMaps are not required to carry information about what has been allocated, only to return the right values. It's perfectly valid to do something like this to preserve memory.

if node_val != default {
  secondary_map.set(node, node_val);
}

(Note: this is automatically done by some impls)

Returning a flag for whether the element was present when deleting it goes kind of against it. For the same reason we do not have a contains(key).

[aborgna-q]

Another problem as you say is that we don't always have V: Eq, so distinguishing already-erased elements may be impossible.

Note: I added a drive-by change to UnmanagedDenseMap::take to avoid allocating memory on some cases.

[acl-cqc]

Yeah OK. I'm a bit meh but don't see anything better as it doesn't seem possible to be consistent with other classes' remove methods :(