Implement PortReader and ChanWriter#10823
Conversation
src/libstd/io/comm_adapters.rs
Outdated
There was a problem hiding this comment.
Indentation for the Rust code base is 4 spaces.
|
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. |
|
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. |
|
Awesome work, thanks for doing this! |
|
Neat! I forgot these types were around and unimplemented. |
|
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. |
src/libstd/io/comm_adapters.rs
Outdated
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
|
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 |
|
@bill-myers That's a good idea. However, when I tried to rustc complained about a lot of conflicting implementations. This works but doesn't seem like the right solution. I might leave that for a separate pull request. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
self.closed can only be true if self.buf is None. That is currently tested in test_port_reader.
There was a problem hiding this comment.
I think all that's left is a squash of commits, then r+ from me.
|
Nice work! One small fix to |
|
Squashed. |
Hi, first pull request here so let me know if I've missed any of the procedure.
Hi, first pull request here so let me know if I've missed any of the procedure.