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 new Ipaddr_cstruct and Macaddr_cstruct libraries #90

Merged
merged 1 commit into from Jul 12, 2019

Conversation

avsm
Copy link
Member

@avsm avsm commented Jul 11, 2019

for conversion to/from cstructs

based on #36

Co-authored-by: Nicolás Ojeda Bär n.oje.bar@gmail.com

for conversion to/from cstructs

based on mirage#36

Co-authored-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@hannesm
Copy link
Member

hannesm commented Jul 12, 2019

this looks good to me, the only nit I have is: could we add an optional ?off:int argument to the functions so we can read&write at offsets other than 0? (otoh, cstructs can be shift/split to the right position)

@avsm
Copy link
Member Author

avsm commented Jul 12, 2019

It simplifies it a lot without the offset argument, and I also figured that cstruct is designed to have efficient shift/splits (with only OCaml minor heap allocation). So I'd prefer to leave it this way rather than repeating elements of the Cstruct interface here unless you strongly prefer to have it.

@avsm
Copy link
Member Author

avsm commented Jul 12, 2019

I'll go ahead and release this, on the basis that adding ?off later is just adding an optional label and so not a huge interface change...

@avsm avsm merged commit 26a7aa8 into mirage:master Jul 12, 2019
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.

None yet

2 participants