Add map and try_map methods to BoundedBTreeMap

[benluelo]

See #11866 for relevant discussion.

I debated on whether to have the signature of the functions passed to map/try_map pass K through, but I realized that this could cause the number of items in the map to decrease due to collisions (i.e. b.map(|(_, v)| (1, v))). While this wouldn't violate any of the guarantees of the datatype, it is odd and most likely unexpected behavior; use .retain().map() if that behavior is desired.

In implementing this, I was wondering if BoundedVec would benefit from this functionality as well. Would a trait make sense, or should it just be a method on the struct directly? I'm not sure if this operation would make sense on a BoundedBTreeSet for the same reason I kept the key as a reference in BoundedBTreeMap::{map/try_map}.

[cla-bot-2021]

User @benluelo, please sign the CLA here.

[ggwpez]

Would a trait make sense, or should it just be a method on the struct directly?

What do you think @KiChjang ? If we want to implement it for other Bounded* containers as well, probably a good idea, or?

[KiChjang]

Would a trait make sense, or should it just be a method on the struct directly?

What do you think @KiChjang ? If we want to implement it for other Bounded* containers as well, probably a good idea, or?

I don't foresee anything wrong with implementing it on bounded types. Perhaps your hesitation comes from the fact that we might change the size of the elements, and hence the bounded size of the overall map? This wouldn't be an issue in my opinion, as the function returns a brand new BoundedBTreeMap, and so can be treated as a completely separate entity as the map that was operated over.

[ggwpez]

I don't foresee anything wrong with implementing it on bounded types. Perhaps your hesitation comes from the fact that we might change the size of the elements, and hence the bounded size of the overall map? This wouldn't be an issue in my opinion, as the function returns a brand new BoundedBTreeMap, and so can be treated as a completely separate entity as the map that was operated over.

Just wanted to hear your opinion, thanks 😄
I guess we can go ahead with this?

[KiChjang]

Wait, I just realized... the map/try_map method are implemented directly as methods on the BoundedBTreeMap type, and I don't think this is required. We could've just implemented a iter_mut() method and gotten all of the map methods for free.

So unfortunately no, I don't think this is actually the right approach to the problem.

[benluelo]

Added iter_mut, let me know what you think

[ggwpez]

So m.map(|(k, v)| 123) now becomes m.iter_mut().for_each(|(k, v)| *k = 123) and for try_ respectively?
Fair enough, then we dont need the dedicated map and try_map functions.

[KiChjang]

Correct, I don't see the need for the try_map/map functions when we can simply get an iterator out of the BoundedBTreeMap.

[KiChjang]

bot merge

[paritytech-processbot]

Waiting for commit status.

[benluelo]

One issue with iter_mut is that it doesn't allow you to transform the item's type in the collection - you can alter the value, but if you want to go from a Map<A, B> to a Map<A, C> you still need to into_iter/map/try_collect/unwrap (which is not ideal if they're bounded by the same S). I think there's usecases for both, personally.