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

Use distinct Flush types for Compress::compress vs Decompress::decompress #79

Closed
dtolnay opened this issue Apr 4, 2017 · 6 comments
Closed

Comments

@dtolnay
Copy link
Member

dtolnay commented Apr 4, 2017

Currently there is a single flate2::Flush type that is used by both flate2::Compress::compress and flate2::Decompress::decompress.

The methods are different in that compress supports Flush::None, Sync, Partial, Full, Finish while decompress supports Flush::None, Sync, Finish. Neither method supports Flush::Block.

It would be nicer to enforce these restrictions statically by having two different Flush types with only the variants that are allowed in each case.

@SamWhited
Copy link
Contributor

What would happen to the Ops trait in this situation? If you give it an associated type Flush, I don't think you'll be able to use any of the enum values in the Writer implementation. Would Writer be split out into a CompressWriter and DecompressWriter? Trying to make Ops/Writer generic WRT a flush type gets very complicated very quickly; I'm not sure there's a good way to do this without a lot of duplication.

@alexcrichton
Copy link
Member

Eh I'd be fine with a bit of duplication in that respect @SamWhited, that's what we have tests for!

@chrisvittal
Copy link
Contributor

chrisvittal commented May 6, 2017

I'm willing to take care of this. Here's my first pass at a design for this.

  1. Split Flush into FlushCompress and FlushDecompress with
    • pub enum FlushCompress { None, Sync, Partial, Full, Finish }
    • pub enum FlushDecompress { None, Sync, Finish }
  2. Add a private Flush trait that is used to construct FlushCompress or FlushDecompress values.
    • The Flush trait has methods none(), sync(), partial(), full(), and finish(), all returning Self
    • The impl Flush for FlushDecompress panics for partial() and full() as they are not valid for FlushDecompress
  3. Add the associated type Flush: Flush to Ops with the impls for Compress having Flush = FlushCompress and Decompress having Flush = FlushDecompress respectively.
  4. Change all references to Flush::MEMBER to the appropriate Flush constructor method.
    • For example change Flush::Finish in Writer::finish(&mut self) to D::Flush::finish().

To add: My branch is here. I've implemented the design I laid out above, and all existing tests currently pass.

@chrisvittal
Copy link
Contributor

I've been doing more investigation on this, and I have some observations. I've linked example code with it's output showing the behavior.

  1. It is only with miniz.c bindings that having some Flush variants being used causes problems.
    • Specifically Block for compress and Block and Full for decompress
    • With compress, passing it Block can cause a panic.
  2. When compiled with --features zlib, everything works, as far as I can tell.

My question is, @alexcrichton, what would you like to do? I believe that Flush::Block or its replacement should probably just be removed (as in the first comment to this issue), however, should Partial be left in for Decompress?

code
output with default features
output with --features zlib

@alexcrichton
Copy link
Member

Thanks for taking this on @chrisvittal! Was gonna comment here but I'll go take a look at the PR instead :)

@alexcrichton
Copy link
Member

Done in #109

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants