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

Writing enums: pre_assert? #272

Open
bilts opened this issue Jun 19, 2024 · 3 comments
Open

Writing enums: pre_assert? #272

bilts opened this issue Jun 19, 2024 · 3 comments
Labels
confusing-api Oops! Confusion occurred enhancement New feature or request

Comments

@bilts
Copy link

bilts commented Jun 19, 2024

I have code like this:

#[derive(BinRead, BinWrite, Debug)]
#[brw(import(message_type: u8))]
enum MessageData {
  #[br(pre_assert(message_type ==  0x0u8))] Nil {
  },
  ...
}

bw / brw don't allow pre_assert so they can't serialize this. Is there a way to reverse parsing an enum like this? Is it feasible to implement pre_assert for brw?... I don't think using assert is correct.

@csnover csnover added enhancement New feature or request confusing-api Oops! Confusion occurred labels Jun 25, 2024
@csnover
Copy link
Collaborator

csnover commented Jun 25, 2024

Hi, thanks for your report!

There are a couple of things you could do now:

  1. Encapsulate the MessageData inside a struct that has the message_type as a field so they are both written out at the same time.
  2. Add #[bw(magic = 0u8)] to each variant to write the correct magic value for the variant, optionally with an Unknown(#[bw(calc(message_type))] u8) final variant.

Another option might be to map to a tuple at the top level but that would require #39 to be implemented.

I am not sure how to make this API less confusing. Are you passing in message_type instead of using magic because a single variant might be matched by multiple different magic values, or for performance, or for some other reason?

@bilts
Copy link
Author

bilts commented Jun 27, 2024

I'll try refactoring a bit based on the suggestions. I hadn't been using magic because there is tricky common header stuff that goes between message_type and the message (MessageData):

  1. message_type
  2. A u16 indicating how many bytes to read / pad from the message
  3. A u8 of flags. If bit 1 is set, then what gets read is a pointer to a message rather than a message itself, a completely different format
  4. Optionally, based on flags kept elsewhere, a sort index
  5. message

Items 2 and 3 make it hard to see how to parse with binrw just using magic.

It's this mess, for reference: https://docs.hdfgroup.org/hdf5/v1_14/_f_m_t3.html#V2ObjectHeaderPrefix

@kitlith
Copy link
Collaborator

kitlith commented Jun 28, 2024

Sounds like you want approach 1 that csnover laid out, then, and you're using pre_assert + import as you should be for that approach. You might want a helper method on MessageData to return the message_type for a given value. At this point you should not be thinking of serializing message_type as the responsibility of MessageData, but of whatever structure/set of functions that contains MessageData and is passing the message_type argument to MessageData. (in that vein, you should probably use #[br(import(...))] instead of #[brw(import(...))], because MessageData does not need external knowledge to know what it's message type is when writing.)

Good luck with the messy format! It's worth noting here that binrw has unfortunately not really gained good support for writing offsets yet, so that might be a bit of a struggle. (see #4)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confusing-api Oops! Confusion occurred enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants