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

Add optional fixed length to lists #384

Merged
merged 2 commits into from
Sep 19, 2024

Conversation

cpetig
Copy link

@cpetig cpetig commented Aug 1, 2024

Resolves #181

@cpetig
Copy link
Author

cpetig commented Aug 1, 2024

Updated version of MR #304 with the conflicts resolved

@jeffparsons
Copy link

Question from the peanut gallery:

Has this change been approved in principle already — i.e. is it just the implementation that's awaiting review?

@lukewagner
Copy link
Member

Yes, I think I've heard broad agreement on this feature. But it is a good question of why not just merge the PR now, given that it's emoji-gated (so that it's clear it's not implemented everywhere) and not controversial. That's probably a good idea, so I'll do that next week unless anyone suggests otherwise.

@lukewagner
Copy link
Member

Rereading the PR just now, it seems like we'd want a low (but relaxable in the future) upper bound on the fixed length so that bindings generators don't have to think about rare pathological cases (similar to what we did for flags). Maybe 16?

@jeffparsons
Copy link

I'd argue for at least 64 to support the largest variants of SHA-2, SHA-3 and BLAKE3, which are all 512 bits, as list<u8, 64>.

I know this is a very narrow use case, but most of the others that are on my radar are smaller arrays, e.g., IPv6, UUID, coordinates and color values for "graphics stuff", etc.

@lukewagner
Copy link
Member

Yes, that's a good point. Given that I'm overdue from merging after my previous comment and this is a minor refinement, I'll merge this PR now as-is and file a new PR to add the length limits in a bit, and we can agree on 64 or whatever else there.

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.

Fixed length arrays
4 participants