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

Amend I/O reform RFC to address issues with flush #950

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
5 participants
@mzabaluev
Copy link
Contributor

commented Mar 6, 2015

No description provided.

@@ -596,13 +616,17 @@ method, clients of `Read` can easily recover the `push` method.
### `Write`
[Write]: #write

The `Writer` trait is cut down to even smaller size:
The `Writer` trait is cut down to even smaller size, and `flush` is taken

This comment has been minimized.

Copy link
@glaebhoerl

glaebhoerl Mar 7, 2015

Contributor

Writer doesn't really get any smaller - all of the same methods are still there - just refactored.

This comment has been minimized.

Copy link
@jmesmon

jmesmon Mar 7, 2015

note that this is an edit of a previous RFC which removed quite a few methods from Write. The statement you're commenting was preserved from that one.


* A convention on `into_inner` methods on composite writers and `BufStream`
is to flush the underlying writer before returning it. The deep flushing
may be undesirable if the consumer wants to continue writing.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Mar 12, 2015

Member

After taking another look at the implementations, I don't think that this "deep flushing" is happening (at least not in std::io), perhaps this was just a problem in old_io?

This comment has been minimized.

Copy link
@mzabaluev

mzabaluev Mar 15, 2015

Author Contributor

Indeed, it's just that the documentation is vague.

@alexcrichton alexcrichton self-assigned this Mar 12, 2015

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 12, 2015

After thinking about this a bit today, I'm not quite sure that we may want the split. I agree that other abstractions may want a flush method as well, but I'm not sure that this incarnation of Flush is appropriate for all use cases. My biggest concern is that the usage of io::Error may make it inadequate for the general use case. For example std::fmt::Write uses a different error type than I/O. Additionally, delegation of a flush method from one trait to another (if the flush method is duplicated among traits) arguably takes fewer lines to express than an explicit implementation of Flush itself.

I feel like there may be a place for a generic Flush, but I don't find the motivation compelling enough to try to introduce the general trait at this time.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 12, 2015

cc @aturon

@lilyball

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2015

I'm not sure what the use-case of a generic Flush is. I can't think of any reason why a function or type would want to be able to flush() a stream without first having written something to it, and if it knows how to write to the stream then it obviously knows what type the stream is (and therefore is already bound on something like Write).

I also don't see what the benefit is of moving the declaration of fn flush(&mut self) -> io::Result<()> out of the Write trait and into a separate trait. Given the hypothetical TextWriter, having both Write and TextWriter declare fn flush(&mut self) -> io::Result<()> is not code duplication in any meaningful sense. Any implementation of these traits would need to implement flush() regardless of where it's declared, and the fact that both Write and TextWritermay want to have an identical methodfn flush(&mut self) -> io::Result<()>` does not necessarily mean that this is in fact a truly generic flush method that is appropriate for all stream types.

In fact, the only benefit that comes to mind is a type that implements both Write and this hypothetical TextWriter would only need to implement flush() once, instead of twice. But that seems like an edge case that's not worth worrying about, and it's also not necessarily obvious that the implementation would even want to be the same in both cases (I can think of some esoteric stream types that might want the ability to have flushing behavior that differs from Write::flush).

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2015

@alexcrichton:

For example std::fmt::Write uses a different error type than I/O.

There is actually a related issue of its own: std::fmt::Error should expose std::io::Error as a possible cause.

@mzabaluev

This comment has been minimized.

Copy link
Contributor Author

commented Mar 15, 2015

I have filed issue rust-lang/rust#23386 to correct the documentation of into_inner, and #978 to amend the I/O RFC accordingly.
As the generic Flush trait did not get much support, I'm closing this PR.

@mzabaluev mzabaluev closed this Mar 15, 2015

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 16, 2015

std: Stabilize the Write::flush method
The [associated RFC][rfc] for possibly splitting out `flush` has been closed and
as a result there are no more blockers for stabilizing this method, so this
commit marks the method as such.

[rfc]: rust-lang/rfcs#950

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 16, 2015

std: Stabilize the Write::flush method
The [associated RFC][rfc] for possibly splitting out `flush` has been closed and
as a result there are no more blockers for stabilizing this method, so this
commit marks the method as such.

[rfc]: rust-lang/rfcs#950

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 16, 2015

std: Stabilize the Write::flush method
The [associated RFC][rfc] for possibly splitting out `flush` has been closed and
as a result there are no more blockers for stabilizing this method, so this
commit marks the method as such.

[rfc]: rust-lang/rfcs#950

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 17, 2015

Rollup merge of rust-lang#23415 - alexcrichton:stabilize-flush, r=aturon
 The [associated RFC][rfc] for possibly splitting out `flush` has been closed and
as a result there are no more blockers for stabilizing this method, so this
commit marks the method as such.

[rfc]: rust-lang/rfcs#950

djrenren pushed a commit to djrenren/libtest that referenced this pull request Jan 22, 2019

std: Stabilize the Write::flush method
The [associated RFC][rfc] for possibly splitting out `flush` has been closed and
as a result there are no more blockers for stabilizing this method, so this
commit marks the method as such.

[rfc]: rust-lang/rfcs#950

gnzlbg pushed a commit to rust-lang/libtest that referenced this pull request Mar 2, 2019

std: Stabilize the Write::flush method
The [associated RFC][rfc] for possibly splitting out `flush` has been closed and
as a result there are no more blockers for stabilizing this method, so this
commit marks the method as such.

[rfc]: rust-lang/rfcs#950
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.