-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Support packed structs in std.io.Reader and std.io.Writer #21601
base: master
Are you sure you want to change the base?
Conversation
278899a
to
04a383b
Compare
There seems to be precedent for using Line 322 in d23db94
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code and comments look nice to me. I'm not convinced the packed struct endianness is handled correctly, but I might be looking at it incorrectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR looks good to me. The updated tests look excellent.
I'm still confused about the handling of big-endian packing, but I'm fine to accept its just my expectations that are wrong, not the code.
Yeah, I'd resolve them if I could... According to GitHub docs only PR authors or folks with write access to the repo can resolve a conversation. So in this specific case, all my comments are addressed. Sorry for the extra hassle. |
d2d0b8f
to
8a17490
Compare
given that packed struct equality operator just got merged #21679 |
intended to close #12960
successor to #21562
Summary
This PR adds support for packed structs in
std.io.Reader
andstd.io.Writer
.Some rationale:
@bitSizeOf
that is a multiple of eight because the user is using the reader / writer on a byte stream, and should be expected to add padding fields to the packed struct if they wish to have padding to align to bytes.std.mem.reverse
instead of byteswap on the backing integer because I think it declares my intent more precisely: to reverse the bytes that underly the packed struct. It declares explicitly that I am relying on the big endian representation being the bytes reversal of the little endian representation. Note that this implementation may need to be changed if packed struct can later contain arrays. Packed structs can't contain arrays #12547Questions
1. Should I be using@compileError
as I am in the switches? Or perhaps unreachable?Test Plan
The unit tests added with this PR should pass.
Feedback welcome, as always!