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

Support packed structs in std.io.Reader and std.io.Writer #21601

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

Conversation

kj4tmp
Copy link
Contributor

@kj4tmp kj4tmp commented Oct 5, 2024

intended to close #12960

successor to #21562

Summary
This PR adds support for packed structs in std.io.Reader and std.io.Writer.

Some rationale:

  1. I chose to not read/write padding bytes of packed structs because I think that most users are using packed structs to avoid padding, and thus the padding due to alignment would be annoying.
  2. I chose to restrict packed struct parameters to having a @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.
  3. For endianness conversion, I chose to use 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 #12547

Questions
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!

@kj4tmp kj4tmp force-pushed the packed-structs-in-reader-writer branch from 278899a to 04a383b Compare October 5, 2024 05:38
@kj4tmp
Copy link
Contributor Author

kj4tmp commented Oct 5, 2024

There seems to be precedent for using @compileError like I am. For example:

if (mask_info != .int) @compileError("ArrayBitSet can only operate on integer masks, but was passed " ++ @typeName(MaskIntType));

Copy link
Contributor

@rootbeer rootbeer left a 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.

lib/std/io/Reader.zig Show resolved Hide resolved
lib/std/io/Reader.zig Show resolved Hide resolved
lib/std/io/Reader.zig Outdated Show resolved Hide resolved
lib/std/io/Writer/test.zig Show resolved Hide resolved
lib/std/io/Reader.zig Show resolved Hide resolved
Copy link
Contributor

@rootbeer rootbeer left a 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.

lib/std/io/Reader/test.zig Show resolved Hide resolved
lib/std/io/Reader.zig Show resolved Hide resolved
@kj4tmp
Copy link
Contributor Author

kj4tmp commented Oct 6, 2024

@rohlem @rootbeer I'm not sure whether I should be clinking "resolve conversation". I would generally expect the conversation creator to click resolve when they feel it is resolved? Might be worth a mention in the contributing guidelines.

@rootbeer
Copy link
Contributor

rootbeer commented Oct 6, 2024

@rohlem @rootbeer I'm not sure whether I should be clinking "resolve conversation". I would generally expect the conversation creator to click resolve when they feel it is resolved? Might be worth a mention in the contributing guidelines.

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.

@kj4tmp kj4tmp force-pushed the packed-structs-in-reader-writer branch from d2d0b8f to 8a17490 Compare October 12, 2024 02:50
@kj4tmp kj4tmp marked this pull request as draft October 14, 2024 00:47
@kj4tmp
Copy link
Contributor Author

kj4tmp commented Oct 14, 2024

given that packed struct equality operator just got merged #21679
I will convert my tests to use testing.expectEqual instead of testing.expectEqualDeep, either as soon as I can finish building from source or get my hands on today's nightly.

@kj4tmp kj4tmp marked this pull request as ready for review October 14, 2024 02:50
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.

Disallow reader.readStruct for packed structs
3 participants