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

std: Implement stdio for std::io #22797

Merged
merged 1 commit into from
Mar 2, 2015
Merged

Conversation

alexcrichton
Copy link
Member

This is an implementation of RFC 899 and adds stdio functionality to the new
std::io module. Details of the API can be found on the RFC, but from a high
level:

  • io::{stdin, stdout, stderr} constructors are now available. There are also
    *_raw variants for unbuffered and unlocked access.
  • All handles are globally shared (excluding raw variants).
  • The stderr handle is no longer buffered.
  • All handles can be explicitly locked (excluding the raw variants).

The print! and println! machinery has not yet been hooked up to these
streams just yet. The std::fmt::output module has also not yet been
implemented as part of this commit.

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

Note that this PR is not intended to be the final result of these functions. This will likely land ahead of the RFC but only because it facilitates migration to std::io where it was not previously possible. These APIs are highly likely to change and the RFC is free to diverge from this implementation (and it probably will!). I will file follow-up PRs to keep the API up-to-date with the RFC.

cc rust-lang/rfcs#899

r? @aturon API-wise
r? @retep998, @nagisa I'd like another pair of eyes on the windows implementation (and unix!)

@alexcrichton
Copy link
Member Author

Ah I will try to add some tests soon as well. I'll both add some independent ones as well as migrate some existing run-pass/bench tests to this API.

fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
self.inner.write(&buf[..cmp::min(buf.len(), OUT_MAX)])
}
fn flush(&mut self) -> io::Result<()> { Ok(()) }
Copy link
Member

Choose a reason for hiding this comment

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

I believe you should just go ahead and do self.inner.flush() here, even if the inner returns Ok(()) anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

The inner doesn't actually implement flush in this case. Each of the underlying primitives implement write as an inherent method (taking &self) so there's no flush method to call.

Copy link
Member

Choose a reason for hiding this comment

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

It does implement it at https://github.com/rust-lang/rust/pull/22797/files#diff-c7c55a739480d5cc82ecd059127d62a9R78. I just generally prefer seeing wrapper structures like StderrLock not assume anything about the object it wraps and just pass the responsibility of doing what’s right to the object it wraps over.

Anyway that’s just a nit and general style preference, so you’re free to ignore it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gah sorry, was thinking of the wrong thing, I will definitely delegate!

@nagisa
Copy link
Member

nagisa commented Feb 25, 2015

I’ve finished my pass over it 😉

@aturon
Copy link
Member

aturon commented Feb 25, 2015

API looks fine to me. I may want to make a few tweaks but we can work this out on the RFC. Thanks @alexcrichton!

@aturon aturon mentioned this pull request Feb 25, 2015
91 tasks
@alexcrichton
Copy link
Member Author

I've pushed an update that removes all *_raw and *Raw methods/primitives due to concerns about the "raw-ness" on Windows. I will delegate these primitives to the RFC instead for now. Otherwise the patch remains the same.

I have also now updated with some usage in tests as well (and will add some more over time as well).

@aturon
Copy link
Member

aturon commented Feb 27, 2015

@bors: r+ 5c0ed68

@alexcrichton
Copy link
Member Author

@bors: r=aturon 1c32ac7

@bors
Copy link
Contributor

bors commented Feb 27, 2015

⌛ Testing commit 1c32ac7 with merge a8b753b...

@bors
Copy link
Contributor

bors commented Feb 27, 2015

💔 Test failed - auto-mac-32-opt

@alexcrichton
Copy link
Member Author

@bors: retry

@bors
Copy link
Contributor

bors commented Feb 28, 2015

⌛ Testing commit 1c32ac7 with merge be1f11a...

@bors
Copy link
Contributor

bors commented Feb 28, 2015

💔 Test failed - auto-win-32-nopt-t

This is an implementation of RFC 899 and adds stdio functionality to the new
`std::io` module. Details of the API can be found on the RFC, but from a high
level:

* `io::{stdin, stdout, stderr}` constructors are now available. There are also
  `*_raw` variants for unbuffered and unlocked access.
* All handles are globally shared (excluding raw variants).
* The stderr handle is no longer buffered.
* All handles can be explicitly locked (excluding the raw variants).

The `print!` and `println!` machinery has not yet been hooked up to these
streams just yet. The `std::fmt::output` module has also not yet been
implemented as part of this commit.
@alexcrichton
Copy link
Member Author

@bors: r=aturon 94d71f8

bors added a commit that referenced this pull request Mar 1, 2015
This is an implementation of RFC 899 and adds stdio functionality to the new
`std::io` module. Details of the API can be found on the RFC, but from a high
level:

* `io::{stdin, stdout, stderr}` constructors are now available. There are also
  `*_raw` variants for unbuffered and unlocked access.
* All handles are globally shared (excluding raw variants).
* The stderr handle is no longer buffered.
* All handles can be explicitly locked (excluding the raw variants).

The `print!` and `println!` machinery has not yet been hooked up to these
streams just yet. The `std::fmt::output` module has also not yet been
implemented as part of this commit.
@bors
Copy link
Contributor

bors commented Mar 1, 2015

⌛ Testing commit 94d71f8 with merge 0c9b49c...

@bors
Copy link
Contributor

bors commented Mar 1, 2015

💔 Test failed - auto-linux-64-x-android-t

@pnkfelix
Copy link
Member

pnkfelix commented Mar 1, 2015

@bors retry

@bors
Copy link
Contributor

bors commented Mar 1, 2015

@bors
Copy link
Contributor

bors commented Mar 1, 2015

💔 Test failed - auto-win-32-opt

@Manishearth
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Mar 1, 2015

⌛ Testing commit 94d71f8 with merge 13769d8...

@bors
Copy link
Contributor

bors commented Mar 1, 2015

💔 Test failed - auto-linux-64-x-android-t

@sfackler
Copy link
Member

sfackler commented Mar 1, 2015

@bors retry

@bors
Copy link
Contributor

bors commented Mar 1, 2015

@bors
Copy link
Contributor

bors commented Mar 1, 2015

💔 Test failed - auto-linux-64-x-android-t

@Manishearth
Copy link
Member

@bors: retry

@bors
Copy link
Contributor

bors commented Mar 1, 2015

@bors
Copy link
Contributor

bors commented Mar 1, 2015

💔 Test failed - auto-win-64-nopt-t

@Manishearth
Copy link
Member

Er, what?

Second time I've seen that failure today. Looks deterministic.

@sfackler
Copy link
Member

sfackler commented Mar 2, 2015

@bors retry

bors added a commit that referenced this pull request Mar 2, 2015
This is an implementation of RFC 899 and adds stdio functionality to the new
`std::io` module. Details of the API can be found on the RFC, but from a high
level:

* `io::{stdin, stdout, stderr}` constructors are now available. There are also
  `*_raw` variants for unbuffered and unlocked access.
* All handles are globally shared (excluding raw variants).
* The stderr handle is no longer buffered.
* All handles can be explicitly locked (excluding the raw variants).

The `print!` and `println!` machinery has not yet been hooked up to these
streams just yet. The `std::fmt::output` module has also not yet been
implemented as part of this commit.
@bors
Copy link
Contributor

bors commented Mar 2, 2015

⌛ Testing commit 94d71f8 with merge c514205...

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.

10 participants