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

FiniteWriter trait + many important fixes #11

Closed
wants to merge 4 commits into from

Conversation

kvark
Copy link
Collaborator

@kvark kvark commented Feb 11, 2014

FiniteWriter is now implemented for every Writer, and used by the test app to terminate the stream properly.
BWT decoding is now fixed.
Ari ByteDecoder doesn't need to know the output size any more, enabled for the test app.


impl FiniteWriter for io::MemWriter {}
impl FiniteWriter for io::stdio::StdWriter {}
impl<W: Writer> FiniteWriter for io::BufferedWriter<W> {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation in this file is a little off, and I'm not sure that this is the best way to go about this. This trait seems very specific and I don't understand why you have FiniteWriter implementations for various writers, but not various other writers.

Can you explain a little more about why this is necessary? Is this purely because you can't call generic methods from destructors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the tabs, I should be more careful when coding at night.
The implementations here are for the things that we actually use in rust-compress. Ideally, we'd want it to be implemented by default on every Writer, and just write specialization implementations for things like ~FiniteWriter. AFAIK, there is no analogue of C++ template specialization in Rust, is there?

Yes, that code was required in order to work around the destructor limitations. I'm not an IO guru, so any insights from you would be very welcome.

@kvark
Copy link
Collaborator Author

kvark commented Feb 12, 2014

OK, I fixed what I could (Ari tests, shared exposure and tabs). Let me know if there is anything else you'd like to see differently.

@alexcrichton
Copy link
Collaborator

I've come to think that FiniteWriter isn't the best solution for the problem at hand. There are too many limitations to its usage and it's too specific to a small set of writers. Let's merge the bwt and ari fixes for now, but continue to think of a better solution for the app.

@kvark
Copy link
Collaborator Author

kvark commented Feb 13, 2014

Ok, fair enough.

@kvark kvark closed this Feb 13, 2014
@kvark
Copy link
Collaborator Author

kvark commented Feb 13, 2014

BWT and Ari fixes are submitted in PR #12

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.

2 participants