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

Implement AsRawFd for Stdin, Stdout, and Stderr #43459

Merged
merged 4 commits into from Aug 4, 2017
Merged

Conversation

@ids1024
Copy link
Contributor

@ids1024 ids1024 commented Jul 24, 2017

@rust-highfive
Copy link
Collaborator

@rust-highfive rust-highfive commented Jul 24, 2017

r? @brson

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

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jul 25, 2017

r? @alexcrichton

Thanks for the PR! My only point of hesitation here is that these structures are internally buffered (stdin and stdout at least) and AFAIK we don't implement as_raw_fd for the BufReader type. I'd personally prefer to avoid this for now until we eventually expose types like StdinRaw and such, but that needs more work on Windows I believe to get working.

@rust-lang/libs, do others have thoughts?

@rust-highfive rust-highfive assigned alexcrichton and unassigned brson Jul 25, 2017
@sfackler
Copy link
Member

@sfackler sfackler commented Jul 25, 2017

I think we should just add StdinRaw &co now - they're pretty trivial to make.

@@ -109,6 +110,21 @@ impl AsRawFd for net::UdpSocket {
}
}

#[stable(feature = "rust1", since = "1.0.0")]

This comment has been minimized.

@BurntSushi

BurntSushi Jul 25, 2017
Member

I think this should be 1.21.0? (Someone check me on the exact version.)

This comment has been minimized.

@ids1024

ids1024 Jul 25, 2017
Author Contributor

What should feature be? I don't know exactly how this works, since trait implementations aren't behind feature flags.

This comment has been minimized.

@alexcrichton

alexcrichton Jul 25, 2017
Member

You can pick a new feature name for this

@@ -104,6 +106,21 @@ impl AsRawFd for net::UdpSocket {
fn as_raw_fd(&self) -> RawFd { *self.as_inner().socket().as_inner() }
}

#[stable(feature = "rust1", since = "1.0.0")]

This comment has been minimized.

@BurntSushi

BurntSushi Jul 25, 2017
Member

Same here.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jul 25, 2017

@sfackler IIRC the implementation on Windows is pretty nontrivial, in that neither Read nor Write can be implemented.

@sfackler
Copy link
Member

@sfackler sfackler commented Jul 25, 2017

Oh, right. I guess it could live in std::os::unix::io?

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jul 26, 2017

Yeah that seems plausible to me. Either that or we could have std::io::StdoutRaw and it just has no methods other than ::new. All methods would be layered on from extension traits in the OS modules.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jul 27, 2017

@sfackler @BurntSushi do y'all have thoughts on specifically not providing this impl on the existing types? My rationale is that they're buffered so they're not quite as "raw" as you'd like, but I'm curious what others' opinions are.

@BurntSushi
Copy link
Member

@BurntSushi BurntSushi commented Jul 28, 2017

@alexcrichton From my perspective, I think all I really care about is just being able to get the std file descriptors easily. It's purely a convenience. I think I'd be just as happy with them existing on the "raw" types. I do somewhat share your concern that getting a file descriptor on something that is buffered is a little strange, although I think the C function fileno conceptually does the same thing? So there's a little bit of precedent there?

@sfackler
Copy link
Member

@sfackler sfackler commented Jul 28, 2017

I think it is worth having the impls on the buffered versions - we can call out the fact that they are buffered on the impl docs which should help avoid confusion there.

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jul 28, 2017

Ok, that sounds convincing enough to me, let's see abuot landing this and pursuing the raw versions independently.

@ids1024 would you be ok adding AsRawHandle implementations for Windows as well?

@rfcbot fcp merge

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jul 29, 2017

@rfcbot fcp merge

@rfcbot
Copy link

@rfcbot rfcbot commented Jul 29, 2017

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@aturon
Copy link
Member

@aturon aturon commented Aug 1, 2017

(I checked for @brson, who is away)

@rfcbot
Copy link

@rfcbot rfcbot commented Aug 1, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Aug 1, 2017

@bors: r+

Alright, thanks @ids1024!

@bors
Copy link
Contributor

@bors bors commented Aug 1, 2017

📌 Commit a878592 has been approved by alexcrichton

@bors
Copy link
Contributor

@bors bors commented Aug 2, 2017

📌 Commit 578123c has been approved by alexcrichton

@bors
Copy link
Contributor

@bors bors commented Aug 2, 2017

Testing commit 578123c with merge 52cb406...

bors added a commit that referenced this pull request Aug 2, 2017
Implement AsRawFd for Stdin, Stdout, and Stderr

rust-lang/rfcs#2074
@bors
Copy link
Contributor

@bors bors commented Aug 2, 2017

💔 Test failed - status-appveyor

@aidanhs
Copy link
Member

@aidanhs aidanhs commented Aug 3, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

@bors bors commented Aug 3, 2017

📌 Commit 303488a has been approved by alexcrichton

@bors
Copy link
Contributor

@bors bors commented Aug 3, 2017

Testing commit 303488a with merge d6a3898...

bors added a commit that referenced this pull request Aug 3, 2017
Implement AsRawFd for Stdin, Stdout, and Stderr

rust-lang/rfcs#2074
@bors
Copy link
Contributor

@bors bors commented Aug 3, 2017

💔 Test failed - status-appveyor

@ids1024 ids1024 force-pushed the ids1024:asrawfd branch from 303488a to eac01f1 Aug 3, 2017
@aidanhs
Copy link
Member

@aidanhs aidanhs commented Aug 3, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

@bors bors commented Aug 3, 2017

📌 Commit eac01f1 has been approved by alexcrichton

@bors
Copy link
Contributor

@bors bors commented Aug 3, 2017

Testing commit eac01f1 with merge 9f5d7a2...

bors added a commit that referenced this pull request Aug 3, 2017
Implement AsRawFd for Stdin, Stdout, and Stderr

rust-lang/rfcs#2074
@bors
Copy link
Contributor

@bors bors commented Aug 3, 2017

💔 Test failed - status-appveyor

@aidanhs
Copy link
Member

@aidanhs aidanhs commented Aug 4, 2017

@bors r=alexcrichton

@bors
Copy link
Contributor

@bors bors commented Aug 4, 2017

📌 Commit 64e426e has been approved by alexcrichton

@bors
Copy link
Contributor

@bors bors commented Aug 4, 2017

Testing commit 64e426e with merge eae446c...

bors added a commit that referenced this pull request Aug 4, 2017
Implement AsRawFd for Stdin, Stdout, and Stderr

rust-lang/rfcs#2074
@bors
Copy link
Contributor

@bors bors commented Aug 4, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing eae446c to master...

@bors bors merged commit 64e426e into rust-lang:master Aug 4, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@malbarbo
Copy link
Contributor

@malbarbo malbarbo commented Aug 9, 2017

@alexcrichton This fixes #40007

@ids1024 ids1024 deleted the ids1024:asrawfd branch Oct 5, 2017
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 3, 2017
Changelog:
Version 1.21.0 (2017-10-12)
==========================

Language
--------
- [You can now use static references for literals.][43838]
  Example:
  ```rust
  fn main() {
      let x: &'static u32 = &0;
  }
  ```
- [Relaxed path syntax. Optional `::` before `<` is now allowed in all contexts.][43540]
  Example:
  ```rust
  my_macro!(Vec<i32>::new); // Always worked
  my_macro!(Vec::<i32>::new); // Now works
  ```

Compiler
--------
- [Upgraded jemalloc to 4.5.0][43911]
- [Enabled unwinding panics on Redox][43917]
- [Now runs LLVM in parallel during translation phase.][43506]
  This should reduce peak memory usage.

Libraries
---------
- [Generate builtin impls for `Clone` for all arrays and tuples that
  are `T: Clone`][43690]
- [`Stdin`, `Stdout`, and `Stderr` now implement `AsRawFd`.][43459]
- [`Rc` and `Arc` now implement `From<&[T]> where T: Clone`, `From<str>`,
  `From<String>`, `From<Box<T>> where T: ?Sized`, and `From<Vec<T>>`.][42565]

Stabilized APIs
---------------

[`std::mem::discriminant`]

Cargo
-----
- [You can now call `cargo install` with multiple package names][cargo/4216]
- [Cargo commands inside a virtual workspace will now implicitly
  pass `--all`][cargo/4335]
- [Added a `[patch]` section to `Cargo.toml` to handle
  prepublication dependencies][cargo/4123] [RFC 1969]
- [`include` & `exclude` fields in `Cargo.toml` now accept gitignore
  like patterns][cargo/4270]
- [Added the `--all-targets` option][cargo/4400]
- [Using required dependencies as a feature is now deprecated and emits
  a warning][cargo/4364]


Misc
----
- [Cargo docs are moving][43916]
  to [doc.rust-lang.org/cargo](https://doc.rust-lang.org/cargo)
- [The rustdoc book is now available][43863]
  at [doc.rust-lang.org/rustdoc](https://doc.rust-lang.org/rustdoc)
- [Added a preview of RLS has been made available through rustup][44204]
  Install with `rustup component add rls-preview`
- [`std::os` documentation for Unix, Linux, and Windows now appears on doc.rust-lang.org][43348]
  Previously only showed `std::os::unix`.

Compatibility Notes
-------------------
- [Changes in method matching against higher-ranked types][43880] This may cause
  breakage in subtyping corner cases. [A more in-depth explanation is available.][info/43880]
- [rustc's JSON error output's byte position start at top of file.][42973]
  Was previously relative to the rustc's internal `CodeMap` struct which
  required the unstable library `libsyntax` to correctly use.
- [`unused_results` lint no longer ignores booleans][43728]

[42565]: rust-lang/rust#42565
[42973]: rust-lang/rust#42973
[43348]: rust-lang/rust#43348
[43459]: rust-lang/rust#43459
[43506]: rust-lang/rust#43506
[43540]: rust-lang/rust#43540
[43690]: rust-lang/rust#43690
[43728]: rust-lang/rust#43728
[43838]: rust-lang/rust#43838
[43863]: rust-lang/rust#43863
[43880]: rust-lang/rust#43880
[43911]: rust-lang/rust#43911
[43916]: rust-lang/rust#43916
[43917]: rust-lang/rust#43917
[44204]: rust-lang/rust#44204
[cargo/4123]: rust-lang/cargo#4123
[cargo/4216]: rust-lang/cargo#4216
[cargo/4270]: rust-lang/cargo#4270
[cargo/4335]: rust-lang/cargo#4335
[cargo/4364]: rust-lang/cargo#4364
[cargo/4400]: rust-lang/cargo#4400
[RFC 1969]: rust-lang/rfcs#1969
[info/43880]: rust-lang/rust#44224 (comment)
[`std::mem::discriminant`]: https://doc.rust-lang.org/std/mem/fn.discriminant.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet