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

Add Reader impl for &[u8] and Writer impl for &mut [u8] #18980

Merged
merged 3 commits into from
Dec 4, 2014

Conversation

erickt
Copy link
Contributor

@erickt erickt commented Nov 15, 2014

This continues the work @thestinger started in #18885 (which hasn't landed yet, so wait for that to land before landing this one). Instead of adding more methods to BufReader, this just allows a &[u8] to be used directly as a Reader. It also adds an impl of Writer for &mut [u8].

@tomjakubowski
Copy link
Contributor

Needs a rebase.

@erickt
Copy link
Contributor Author

erickt commented Nov 21, 2014

Rebased, and ready for a re-review.

@erickt erickt force-pushed the reader branch 2 times, most recently from 30fdb60 to b1a1688 Compare November 21, 2014 04:34
@erickt erickt changed the title Add Reader impl for &[u8] Add Reader impl for &[u8] and Writer impl for &mut [u8] Nov 21, 2014
let write_len = buf.len();
if write_len > self.len() {
return Err(IoError {
kind: io::OtherIoError,
Copy link
Contributor

Choose a reason for hiding this comment

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

couldn't this be a ShortWrite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How does an EndOfFile if self.len() is 0, and ShortWrite if it was able to partially right the file sound?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should writing a &[] to a &[] be an EndOfFile, or be okay?

@sfackler
Copy link
Member

The &mut [u8] impl will cause some weird interactions with write!:

let mut buf = vec![];

write!(&mut buf, "hello world"); // Calls Vec's impl
write(buf, "hello world"); // Calls &mut [u8]'s impl

@erickt
Copy link
Contributor Author

erickt commented Nov 21, 2014

@sfackler: That is... weird. I would have expected write!(buf, "hello world") would have used the Vec impl. I wonder if this is related to #19147? cc @nikomatsakis.

@sfackler
Copy link
Member

@erickt it's due to this line, which was apparently added to force people to pass references in like you would a function: https://github.com/rust-lang/rust/blob/master/src/libstd/macros.rs#L266

@alexcrichton
Copy link
Member

While it's possible to implement Writer on &mut [u8], it may be going a little too far. I think @sfackler's example puts me over the edge in thinking that it's not worth implementing. I personally find it a little odd that memory buffers are I/O objects (albeit making sense technically), and I wouldn't expect a write into a &mut [u8] to be too super common anyway.

Other than that this looks great though, thanks @erickt!

@erickt
Copy link
Contributor Author

erickt commented Nov 27, 2014

I've updated my PR to factor out the Writer impl for &mut [u8] since everything else seems non-confrontational.

@@ -447,12 +447,11 @@ impl<T, E> Result<T, E> {
/// use std::io::{BufReader, IoResult};
///
/// let buffer = "1\n2\n3\n4\n";
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be let mut buffer = b"...";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch.

@alexcrichton
Copy link
Member

Looks great, thanks @erickt! I think there may be a waiting failure in the doctest, but otherwise r=me

@@ -446,13 +446,12 @@ impl<T, E> Result<T, E> {
/// ```
/// use std::io::{BufReader, IoResult};
///
/// let buffer = "1\n2\n3\n4\n";
/// let mut reader = BufReader::new(buffer.as_bytes());
/// let mut buffer = "1\n2\n3\n4\n";
Copy link
Member

Choose a reason for hiding this comment

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

This needs a .as_bytes().

bors added a commit that referenced this pull request Dec 4, 2014
This continues the work @thestinger started in #18885 (which hasn't landed yet, so wait for that to land before landing this one). Instead of adding more methods to `BufReader`, this just allows a `&[u8]` to be used directly as a `Reader`. It also adds an impl of `Writer` for `&mut [u8]`.
@bors bors closed this Dec 4, 2014
@bors bors merged commit 298b525 into rust-lang:master Dec 4, 2014
Antti pushed a commit to Antti/rust-amqp that referenced this pull request Dec 8, 2014
rust-lang/rust#18980
This implements Reader for us.

With this commit we finished using both MemWriter & MemReader.
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.

7 participants