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

Update to zig 0.13.0 #3

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

Conversation

DomKalinowski
Copy link

This PR mainly focuses on getting the library build and tests passing on version 0.13.0.

The only addition is making the library possible to fetch as a dependency like:

zig fetch --save=zig_compact git+https://github.com/nsmryan/zig_sealed_and_compact

Updated `field.field_type` to `field.type` for better consistency with Zig's type system.

Replaced `@ptrCast` with `@as` for pointer conversions and added `const` qualifiers where applicable.

Modified test cases to reflect these changes and ensure correctness.

This refactoring aligns the code with current current stable (0.13.0) version without altering functionality.
@nsmryan
Copy link
Owner

nsmryan commented Sep 30, 2024

Thank you for the PR!
I have a PR updating to zig 12 over at https://codeberg.org/tuplestruct/zig_sealed_and_compact where I do my development these days. I appreciate that you updated the build as well, and I will merge these to come up with a single version at some point.

I just want to caution you about this library (I should put this in the README as well): I found that it doesn't work will with the zig standard library data structures which sometimes have:

  1. pointers or slices within other slices, which this library doesn't detect and results in duplicating memory (both the outer and inner slice get their own allocations) which is incorrect. I suppose this could be detected in principal.
  2. Unusual memory management such as storing data before a pointer and backing up to it to get a structure's metadata. This library has no way of knowing about this extra data. This can't be detected from type information, so there is no generic way to support it.

For the use-case I wrote sealed_and_compact, I instead ended up writing a custom serializer/deserializer that accounts for specific implementation details of standard library functions.
However, there might still be uses for this library, and I think the idea is kind of neat, although I was a bit disappointed to find that I could not use it.

@DomKalinowski
Copy link
Author

DomKalinowski commented Sep 30, 2024

Hi @nsmryan!
No worries, and thank you for creating this library in the first place.

I've addressed some of those problems with slices in my other PR #4.
The next PR covers all my use cases, so I'm happy with some other corner cases that are not working at the moment.

there might still be uses for this library, and I think the idea is kind of neat

It is, and in fact, I looked at two or three other serialisation libraries and even fixed one to learn that recursive structures cannot be resolved due to its "comptime" nature.

It seems zig-protobuf suffers the same issue: Arwalk/zig-protobuf#69 (comment)

=========

Optional PR no. 1

Once you'll be able to go through those two PR's I'd be happy to add a custom test runner so you'll be able to see the results like:
image
This new runner with watchexec makes for a great workflow, when all your tests are re-run on save:

watchexec -c -e zig zig build test --verbose

Optional PR no. 2

I could also add a Github actions config with a badge in README.

=========

You can check my Fork for both options.

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.

2 participants