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

Implement Idx for u8, u16, u32 #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

samestep
Copy link

@samestep samestep commented Jan 28, 2025

For my use case, I have a bunch of IndexVec<I, T> where I: Idx is a custom type, but I also need a few IndexVec<u32, T> as well. This PR implements Idx for the primitive types u8, u16, and u32.

The implementations for u8 and u16 should be uncontroversial. The one for u32 is a bit less clear since technically Rust does support 16-bit platforms, but it's the only one from this PR that I actually need 😅

@thomcc
Copy link
Owner

thomcc commented Jan 29, 2025

I'm not worried about 16 bit platforms, they can just not use that impl. It's been a while since I looked at this crate, but this seems fine, so long as you can make it compile and make tests pass.

@samestep samestep changed the title Implement Idx for u8, u16, u32, usize Implement Idx for u8, u16, u32 Jan 29, 2025
@samestep
Copy link
Author

Sounds good, thanks! I fixed the compilation error by just removing the impl for usize.

@thomcc
Copy link
Owner

thomcc commented Jan 31, 2025

Mind bumping the MSRV to whatever is needed that still works?

@samestep
Copy link
Author

Hmm, I tried reproducing the CI failure locally via cargo +1.61.0 but was unable to do so. I suspect that the failure is unrelated to this PR, though; could you try allowing the CI run on #15 to see if that also fails?

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.

2 participants