use zlib_rs api
#513
use zlib_rs api
#513
Conversation
folkertdev
force-pushed
the
zlib-rs-api
branch
2 times, most recently
from
46ea2c4 to
32a775b
Compare
folkertdev
force-pushed
the
zlib-rs-api
branch
2 times, most recently
from
d34a7b8 to
af1615e
Compare
|
@Byron I think this is ready now in terms of what the code looks like. Could you give this a once-over to see that it looks allright from an API perspective? Then we'll release |
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
Thanks so much, this looks fantastic.
It seems that integrating zlib-rs natively is very straightforward, and I'd live to see this PR merged.
So I am looking forward to doing that, just let me know.
| @@ -0,0 +1,164 @@ | |||
| //! Implementation for `zlib_rs` rust backend. | |||
There was a problem hiding this comment.
What would help me here is a quick idea of why this module exports what it exports. My guess is that these types are used to integrate with flate2, and it expects these primitives to exist and implemented by each backend.
It's probably stating the obvious then 😅, and maybe there can be more in-depth information on what makes this module a little bit special even. Are there implications for allocations? Does it allocate? I guess I'd be happy to ready about everything you were thinking at the time of writing so it's conserved.
There was a problem hiding this comment.
I wrote some things but yeah generally it's just the most bare-bones thing that satisfies the assumptions that are made about a backend.
folkertdev
force-pushed
the
zlib-rs-api
branch
from
af1615e to
50f9913
Compare
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
Absolutely fantastic, and thanks for these wonderful docs 🙏.
| //! Every backend must provide two types: | ||
| //! | ||
| //! - `Deflate` for compression, implements the `Backend` and `DeflateBackend` trait | ||
| //! - `Inflate` for decompression, implements the `Backend` and `InflateBackend` trait | ||
| //! | ||
| //! Additionally the backend provides a number of constants, and a `ErrorMessage` type. | ||
| //! | ||
| //! ## Allocation | ||
| //! | ||
| //! The (de)compression state is not boxed. The C implementations require that the z_stream is | ||
| //! pinned in memory (has a fixed address), because their z_stream is self-referential. The most | ||
| //! convenient way in rust to guarantee a stable address is to `Box` the data, but it does add an | ||
| //! additional allocation. | ||
| //! | ||
| //! With zlib_rs the state is not self-referential and hence no boxing is needed. The `new` methods | ||
| //! internally do allocate space for the (de)compression state. |
There was a problem hiding this comment.
Lovely! This gives me the background about some fundamentals of the crate that I should have obtained years ago 😅.
|
Oh wow, I was close to merging this. Let's put the update in a separate commit as well please, it's easier to review if we stop rewriting now. |
folkertdev
force-pushed
the
zlib-rs-api
branch
from
50f9913 to
5b24d98
Compare
|
We've released 0.5.3, containing the stable API. This is ready to merge now, and if there are no issues we can make it the default implementation. |
I missed that. What do you mean with the update? just the |
|
Sorry, I mean to ask to not force-push anymore as I have to re-review everything in the worst case. Which might have happened. Besides that, I am glad this will be ready. And a version bump to |
src/ffi/miniz_oxide.rs
Outdated
| fn make(level: Compression, zlib_header: bool, _window_bits: u8) -> Self { | ||
| // Check in case the integer value changes at some point. | ||
| debug_assert!(level.level() <= 10); | ||
| debug_assert!(level.level() <= 9); |
There was a problem hiding this comment.
Why is this change necessary? miniz_oxide supports level 10 according to its docs.
There was a problem hiding this comment.
Weird, I had assumed that it was a typo. Let me fix that then
3 participants
This is an initial design for using the
zlib_rsAPI, calling the safe functions that it exposes rather than the unsafe wrappers oflibz-rs-sys.I'm designing the flate2 side here, the actual implementation still uses
libz-rs-sysbecausezlib-rsdoes not export some things that we need here.Inspired by GitoxideLabs/gitoxide#2155.