Can a dev review my Rust bindings?

[ebiggers]

Can a dev review my Rust bindings?

[ebiggers]

I took a quick look, and it looks sane.

Maybe Compressor::new() should pass failures to the caller rather than panicing, given that a NULL return from libdeflate_alloc_compressor() can mean that the compression level is invalid? I.e. it doesn't always mean "out of memory".

Is the omission of bindings for the *_decompress_ex() functions (which have the actual_in_nbytes_ret argument) intentional? Likewise for the Adler32 and CRC-32 functions.

Also a few typos I happened to notice:

• panic message in decompress_deflate() has wrong function name
• panic message in Compressor::new() has wrong function name
• comment above compress_zlib() says out_deflate_data instead of out_zlib_data
• comment above compress_gzip() says out_deflate_data instead of out_gzip_data

[adamkewley]

Thanks for this @ebiggers , it is very valuable to have another dev look at this.

To deal with the fact that a NULL might be thrown by libdeflate_alloc_compressor() because the compression level is invalid, I moved returning a result into CompressionLvl::new(i32), so the signature for that is now: pub fn new(level: i32) -> CompressionLevelResult. This means that library users should not be able to construct an arbitrary CompressionLvl without the number being range-checked.

I decided to add the Result at this point, rather than in ::new to make it clear, from a type-system POV, that the only thing a library user can do erroneously is provide an invalid compression level. Logically, there are no other side-effects to allocating a compressor other than memory allocation, and returning a Result implies that there are either invalid function arguments or side-effects (e.g. IO) associated with allocation. The commit for that change is here:

adamkewley/libdeflater@75dae06

Amazingly well-spotted on the typos :D, which are now fixed:

adamkewley/libdeflater@b410db1

As for omitting the _decompress_ex() and crc functions, it is mostly because this is a first version that has the minimal API required that covers normal use-cases (e.g. loading a whole zlib blob). The _decompress_ex() functions feel like they are targeted to more specific use-cases, such as loading a blob you "know" is chunked within some tolerance limit but where you don't know the exact chunk sizes up-front. I will add these methods the next time I get an hour or two to implement the wrappers (+ docs, tests, etc. etc.): again, I think they're useful but weren't the focus of this first version.

On another note, for the benchmarks, they're still not fully-formed yet because there's no comparison between the compression ratio of flate2's default compressors and libdeflate's default compression level, but the results do look promising for this particular corpus (I'll need "bad case" files, such as large files, for comparison)

[adamkewley]

Just as an update on this:

• I added the crc32 functions to the API (adamkewley/libdeflater@5b95efd)
• All methods have been renamed to match libdeflate (adamkewley/libdeflater@0d14ddb)
• Benchmarks have been updated with Canterbury corpus. Libdeflate is around ~2-3x faster on average (adamkewley/libdeflater@18c8278)

My intended next step for this is to throw the bindings out to the wider Rust community for more feedback. If it gains attention, then I'll also add the _ex and adler32 functions as people ask for them. My main goal is to have a working first version of the bindings then adjust as things move forward!

[ebiggers]

Closing since I reviewed this earlier. Let me know if there's anything else you want me to take a look at or if you have any other questions.