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.

Sign up for GitHub

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

Jump to bottom

Implement BinWrite for PosValue #270

Closed
amirbou opened this issue Jun 15, 2024 · 1 comment
Closed

Implement BinWrite for PosValue #270

amirbou opened this issue Jun 15, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@amirbou
Copy link
Contributor

amirbou commented Jun 15, 2024

Hey, currently PosValue does not implement BinWrite, which prevents it from being used in structs using #[binrw].

On my machine I implemented (for PosValue<T>):

  • BinWrite - by ignoring the pos field
  • From<T> - by setting pos=0, making construction easier
  • Default if T: Default - by setting pos=0, allowing #[derive(Default)] on structs using PosValue

I added some asserts in pos_value.rs to check everything, and it seems to be working correctly.

May I go ahead and open a PR with the changes?

Thanks!
@csnover
Copy link
Collaborator

csnover commented Jun 25, 2024

Hi, thanks for your report! Implementing BinWrite for PosValue<T> seems reasonable to me since this is just a wrapper type for position metadata. I feel less certain about adding From<T> and Default implementations but can’t think of anything off the top of my head that would be hurt by it. Go ahead and open a PR any time!

@csnover csnover added the enhancement New feature or request label Jun 25, 2024
@amirbou amirbou mentioned this issue Jun 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Feature - BinWrite for PosValue<T> amirbou/binrw
2 participants
@csnover @amirbou