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

[WIP] Use miniz_oxide directly in rust backend #202

Merged
merged 6 commits into from
Aug 14, 2019
Merged

[WIP] Use miniz_oxide directly in rust backend #202

merged 6 commits into from
Aug 14, 2019

Conversation

oyvindln
Copy link
Contributor

@oyvindln oyvindln commented Aug 7, 2019

This avoids the C API and it's unsafeness entirely.

Can probably further simplify a bit by altering the Stream struct and direction trait StreamWrapper doesn't have to be an enum, neither are public.

Needs some thorough testing.

fixes #181

@alexcrichton
Copy link
Member

Thanks for this! One thing I'd like to do here is to reduce the number of #[cfg] necessary. Would it be possible to push more of the #[cfg] into the src/ffi.rs module itself? That way the backend-specific implementations could all be located there and ideally src/mem.rs is free from #[cfg] (largely at least)

@oyvindln
Copy link
Contributor Author

oyvindln commented Aug 9, 2019

I'll look into it.

@oyvindln
Copy link
Contributor Author

Right, pushed all of the back-end specific stuff into ffi.rs. Only ones that remain are the zlib-specific public functions.

Is there some specific reason the zlib mz_stream is boxed, while the miniz one is not? Otherwise they could share that code.

@alexcrichton
Copy link
Member

That's actually probably a bug, want to go ahead and box both of them up? I think there's also an error on nightly to handle?

@oyvindln
Copy link
Contributor Author

oyvindln commented Aug 13, 2019

I'll box both then.

The nightly error may actually be something that's in the original code, the lint was added recently, and the original code uses mem::zeroed() here too. I suspect it's the extern fn fields in z_stream that the compiler does not want to assume zero is a valid value for. In the miniz variant, those fields are wrapped in option.

@alexcrichton
Copy link
Member

Hm yeah so I think I messed that up when I wrote libz-sys.

If you wouldn't mind adding #[allow(invalid_value)] for now I can try to clean this up after this lands.

Ignore warning so build works on nightly while bug in libz is not fixed
@alexcrichton alexcrichton merged commit 0954103 into rust-lang:master Aug 14, 2019
@alexcrichton
Copy link
Member

👍

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.

Directly use miniz_oxide
2 participants