Exposed deflateSetDictionary and inflateSetDictionary functionality when using zlib #74
Conversation
|
Thanks for the PR! I'm somewhat wary about making this zlib only, but it seems fundamentally incompatible (or unimplemented) with miniz, so in that sense seems fine! Would it be possible to avoid extending the error enum for now? That way we could land this immediately, otherwise it'll need to wait until 1.0 |
|
I'd love to have access to dictionary API too. @alexcrichton is your question still the primary blocker of this PR? @Lukazoid do you mind either updating it or may I use your branch as base for a new PR? |
|
@RReverser ah yeah now that this crate has hit 1.0 it'd need to be added in a backwards-compatible fashion, but I'd still be ok landing! |
|
It should be relatively simple to add support for this in the rust back-end later as well. |
|
@alexcrichton To clarify: by "backwards-compatible fashion" you mean that extending If so, do you mind if I make this part of |
|
@RReverser yeah, but exposing it as an error sounds great! |
|
Sorry I never got back to you when I originally made this PR @alexcrichton. If @RReverser hasn't implemented this functionality already, I can make those changes (remove the added |
|
@Lukazoid I haven't gotten to it either (beyond some local changes to bring it up-to-date with master), so feel free to update your PR - that's even better. |
src/mem.rs
Outdated
| ffi::inflateSetDictionary(stream, dictionary.as_ptr(), dictionary.len() as ffi::uInt) | ||
| }; | ||
|
|
||
| assert_eq!(rc, ffi::MZ_OK); |
There was a problem hiding this comment.
I was thinking it would be better to return Result<u32, DecompressError> from this function so that error in case of incorrect dictionary or in case of calling set_dictionary at incorrect time (it has to be called only immediately after Z_NEED_DICT) could be still handled gracefully.
There was a problem hiding this comment.
I believe I originally implemented these methods before flate2 returned Result and simply had asserts against MZ_OK.
Returning an actual Result is definitely better, I assume it's safe to presume the adler code will already be populated from a previous call to inflate. So if inflateSetDictionary works without error I can return that.
src/mem.rs
Outdated
| ffi::deflateSetDictionary(stream, dictionary.as_ptr(), dictionary.len() as ffi::uInt) | ||
| }; | ||
|
|
||
| assert_eq!(rc, ffi::MZ_OK); |
There was a problem hiding this comment.
Maybe return Result at least as Result<u32, ()>?
There was a problem hiding this comment.
As above, I think I was following the old style with assertions. I will make this change this evening.
|
This looks awesome, thanks so much @Lukazoid! |
The functionality is currently only available when using the
CompressandDecompresstypes, deciding upon a usable API for the IO types requires a fair bit more thought and may be something to consider in future.