Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Implement Sample for byte arrays #35

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

HEnquist
Copy link
Contributor

@HEnquist HEnquist commented Nov 2, 2023

This implements the Sample trait for byte arrays of length 2, 3, 4, 8 and 16, which should cover the byte representation of all common numerical formats. I added a simple test to show it working, not sure if it's placed where it belongs.

@udoprog
Copy link
Owner

udoprog commented Nov 3, 2023

Cheers! Any particular reason you don't want to make it generic over its length?

@udoprog udoprog added the enhancement New feature or request label Nov 3, 2023
@HEnquist
Copy link
Contributor Author

HEnquist commented Nov 3, 2023

No reason except that I didn't think of it 😋

@HEnquist
Copy link
Contributor Author

HEnquist commented Nov 3, 2023

Does Sample really need Default?
I tried redoing the byte arrays with const generics and got stuck on missing Default since there is no generic implementation for arrays yet, only for a number of fixed lengths (up to 32 I think).
I tried just removing that Default bound here:

pub unsafe trait Sample: Copy + Default {

All tests still pass. Am I missing something or could that Default stay gone?

@udoprog
Copy link
Owner

udoprog commented Nov 4, 2023

Feel free to remove it. I think the ZERO associated type already serves that purpose and is more correct. All though its documentation is not great.

@HEnquist
Copy link
Contributor Author

HEnquist commented Nov 5, 2023

Good, Default stays gone then. I pushed a new version of this with generic length.

@udoprog udoprog merged commit fe5e782 into udoprog:main Nov 6, 2023
8 checks passed
@udoprog
Copy link
Owner

udoprog commented Nov 6, 2023

Cheers!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants