Skip to content
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

Make collections::VecMap generic over its key type #19715

Closed
wants to merge 1 commit into from

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Dec 10, 2014

In order to accomplish this, a new trait UintKey is introduced that allows
converting arbitrary types to uint indices and back.

As this adds a new type parameter to VecMap, this is a [breaking-change]. To
fix it, either annotate one of your insertions or set the key type to uint in
a different way.

This also fixes the previously broken PartialEq and Eq implementation of
VecMap.

In order to accomplish this, a new trait `UintKey` is introduced that allows
converting arbitrary types to `uint` indices and back.

As this adds a new type parameter to `VecMap`, this is a [breaking-change]. To
fix it, either annotate one of your insertions or set the key type to `uint` in
a different way.

This also fixes the previously broken `PartialEq` and `Eq` implementation of
`VecMap`.
@Gankra
Copy link
Contributor

Gankra commented Dec 10, 2014

Can you provide a motivation for this? CC @aturon

@tbu-
Copy link
Contributor Author

tbu- commented Dec 10, 2014

The motivation for this is that I can use my own, newtyped struct as a index, in a way similar to other collections providing a map functionality.

@lifthrasiir
Copy link
Contributor

FromPrimitive and ToPrimitive might be a better choice, if you allow the key conversion to be able to panic.

@Gankra
Copy link
Contributor

Gankra commented Dec 11, 2014

Honestly this will probably have to go through some sort of RFC. If we're interested in this sort of pattern we could use it with Bitv and Trie as well.

@aturon
Copy link
Member

aturon commented Dec 11, 2014

This is an interesting idea, and would bring the API into closer alignment with other maps. I'd want to default the type parameter to uint, so that you can ignore this generality most of the time.

Using defaults would make this a largely backwards-compatible change, except that the key parameter wants to come before the value parameter...

I do agree with @gankro -- as the libraries are stabilizing, changes like this increasingly need to go through the RFC process. Would you mind raising some discussion on this point on this RFC? We could probably add it in if there's broad support.

@tbu-
Copy link
Contributor Author

tbu- commented Dec 23, 2014

Superseded by #20150.

@tbu- tbu- closed this Dec 23, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants