Skip to content

Implement PortReader and ChanWriter #10823

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

Merged
merged 1 commit into from
Dec 10, 2013
Merged

Implement PortReader and ChanWriter #10823

merged 1 commit into from
Dec 10, 2013

Conversation

rapha
Copy link
Contributor

@rapha rapha commented Dec 5, 2013

Hi, first pull request here so let me know if I've missed any of the procedure.


struct PortReader<P>;
struct PortReader<P> {
buf: ~[u8],
Copy link
Member

Choose a reason for hiding this comment

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

Indentation for the Rust code base is 4 spaces.

@rapha
Copy link
Contributor Author

rapha commented Dec 5, 2013

Sorry, not sure what the best way is to reply to inline comments.

I've addressed the issues you raised and squashed it back into a single commit.

@rapha
Copy link
Contributor Author

rapha commented Dec 5, 2013

Oops. Just realized that buffering this way will typically cause the buffer to grow endlessly. I guess a chunked buffer that can release chunks as they are read would be better.

@alexcrichton
Copy link
Member

Awesome work, thanks for doing this!

@brson
Copy link
Contributor

brson commented Dec 5, 2013

Neat! I forgot these types were around and unimplemented.

@huonw huonw mentioned this pull request Dec 6, 2013
@rapha
Copy link
Contributor Author

rapha commented Dec 6, 2013

So, I've updated the code with more doc, changed the way it buffers to reduce copying, and fixed eof.

Sorry, I realise I shouldn't have been continually squashing my changes back into a single commit, as this breaks the whole pull request mechansim.

Please take another look.

chan.send(~[1,2]);
chan.send(~[3,4]);

task::spawn_with(port, |port| {
Copy link
Member

Choose a reason for hiding this comment

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

Does this work on a recent master? I believe with the move to use proc()'s, spawn_with is now unnecessary (and is removed), i.e.

task::spawn(proc() {
   let mut reader = PortReader::new(port);
   // etc.
})

should work fine.

Copy link
Member

Choose a reason for hiding this comment

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

This should also invert the sender/receiver. Right now failures in spawned tasks won't trigger test failures, so the asserts should happen in the test task (unless you have a channel waiting for the test task to exit, in which case a failure would close the channel causing a test failure).

@bill-myers
Copy link
Contributor

How about generalizing this to work on an Iterator instead of a Port, and implementing Iterator for Port?

Then one could use it for a [[u8]] as well, for instance.

@rapha
Copy link
Contributor Author

rapha commented Dec 9, 2013

@bill-myers That's a good idea. However, when I tried to

impl<P: GenericPort<~[u8]>> iter::Iterator<~[u8]> for P

rustc complained about a lot of conflicting implementations. This works

impl iter::Iterator<~[u8]> for Port<~[u8]>

but doesn't seem like the right solution.

I might leave that for a separate pull request.


fn eof(&mut self) -> bool { fail!() }
fn eof(&mut self) -> bool { self.closed }
Copy link
Member

Choose a reason for hiding this comment

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

I think this should also factor in whether self.buf is None vs Some and where pos is relative to the array inside Some. Could you add a test for this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.closed can only be true if self.buf is None. That is currently tested in test_port_reader.

Copy link
Member

Choose a reason for hiding this comment

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

good point!

Copy link
Member

Choose a reason for hiding this comment

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

I think all that's left is a squash of commits, then r+ from me.

@alexcrichton
Copy link
Member

Nice work! One small fix to eof and r+ by me. Also, would you mind rebasing these commits into a single commit? Thanks again!

@rapha
Copy link
Contributor Author

rapha commented Dec 9, 2013

Squashed.

bors added a commit that referenced this pull request Dec 9, 2013
Hi, first pull request here so let me know if I've missed any of the procedure.
@bors bors closed this Dec 10, 2013
@bors bors merged commit 7168d71 into rust-lang:master Dec 10, 2013
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.

6 participants