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

Encode/decode to/from references to readers #2

Merged
merged 1 commit into from Oct 10, 2014

Conversation

@tedsta
Copy link
Contributor

commented Oct 9, 2014

I borrowed the pattern used in the json encoder in the standard library. The code uses an unsafe block, but I guess it can be removed whenever rust-lang/rust#14302 is resolved. All the tests pass at least :P

@TyOverby

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2014

Looks good! Thanks for the pull request! Before I merge, why is R forced to implement Buffer? I'd like to keep the constraints as low as possible, but if you have a good reason, I'll merge this.

Also, do you want to be added as a contributor?

}

impl <R: Reader> DecoderReader<R> {
pub fn new(r: R) -> DecoderReader<R> {
impl<'a, R: Reader+Buffer> DecoderReader<'a, R> {

This comment has been minimized.

Copy link
@TyOverby

TyOverby Oct 10, 2014

Collaborator

Why is this R requiring Buffer?

@tedsta

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2014

std::io::Reader by itself doesn't have a read_char method, which was needed here: https://github.com/TyOverby/binary-encode/pull/2/files#diff-fe8c5b0f0b5c8a2045f912ca4aaaefc7L74

Before, the code got around that by using a BufferedReader. Luckily, MemReader implements Buffer, which does have a read_char method. Now, the user can choose whether or not to use a buffered reader. I think that's a nice property?

No need to add me as a contributor, I'll just do the pull request thang 'cause it's fun.

Thanks for writing this btw! They should put something like this in the standard library, especially since they even included json. This library is going to clean up a ton of binary serialization code in my project.

@TyOverby

This comment has been minimized.

Copy link
Collaborator

commented Oct 10, 2014

Ok, yeah, that makes sense! Thanks for your contribution! I'll merge this
when I get home.

On Thursday, October 9, 2014, Theodore DeRego notifications@github.com
wrote:

std::io::Reader by itself doesn't have a read_char method, which was
needed here:
https://github.com/TyOverby/binary-encode/pull/2/files#diff-fe8c5b0f0b5c8a2045f912ca4aaaefc7L74

Before, the code got around that by using a BufferedReader. Luckily,
MemReader implements Buffer, which does have a read_char method. Now, the
user can choose whether to use a buffered reader. I think that's a nice
property?

No need to add me as a contributor, I'll just do the pull request thang
'cause it's fun.

Thanks for writing this btw! They should put something like this in the
standard library, especially since they even included json. This library is
going to clean up a ton of binary serialization code in my project.


Reply to this email directly or view it on GitHub
#2 (comment).

-Ty Overby
:wq

TyOverby added a commit that referenced this pull request Oct 10, 2014

Merge pull request #2 from tedsta/master
Encode/decode to/from references to readers

@TyOverby TyOverby merged commit 413ffac into servo:master Oct 10, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.