Skip to content

Initial support for serialization.#16

Merged
GreyCat merged 1 commit into
kaitai-io:masterfrom
jchw-forks:basic-write-support
Sep 10, 2019
Merged

Initial support for serialization.#16
GreyCat merged 1 commit into
kaitai-io:masterfrom
jchw-forks:basic-write-support

Conversation

@jchv

@jchv jchv commented Sep 8, 2019

Copy link
Copy Markdown
Contributor

In a similar vein to kaitai-io/kaitai_struct_cpp_stl_runtime#30, this adds some of the most basic write primitives that would be needed for implementing basic serialization of seqs in Kaitai Struct.

@GreyCat

GreyCat commented Sep 9, 2019

Copy link
Copy Markdown
Member

I can't help noticing that you're using a per-stream-allocated buffer, which potentially can be a problem using this in multithreaded environment (although probably in reality it's not, as one still relies on write calls to be sequential to have any meaningful result).

Thanks for this contribution! Hope somebody more proficient in go than me can take a look :)

@jchv

jchv commented Sep 9, 2019

Copy link
Copy Markdown
Contributor Author

I only did this because the reader is doing the same thing, and it should lead to a reasonable performance benefit for single thread since it will reduce allocations during writing (versus allocating a new buffer per function call.) But yeah, I figure simultaneous multithreading with the same writer is useless due to the issue of only having one cursor; it would only ever interleave and corrupt anyways. So unless I have misunderstood, this should be perfectly safe to do, I believe.

@GreyCat
GreyCat requested a review from cugu September 9, 2019 19:19

@cugu cugu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to merge. Multithreading is not handled currently anyway.

@GreyCat
GreyCat merged commit b296c90 into kaitai-io:master Sep 10, 2019
@GreyCat

GreyCat commented Sep 10, 2019

Copy link
Copy Markdown
Member

Merging in, thanks, everyone :)

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.

3 participants