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

impl AsFd and AsRawFd for io::{Stdin, Stdout, Stderr}, not the sys versions #102847

Merged

Conversation

joshtriplett
Copy link
Member

#100892 implemented AsFd for the
sys versions, rather than for the public types. Change the
implementations to apply to the public types.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 9, 2022
@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 9, 2022
@joshtriplett
Copy link
Member Author

Noticed when trying to merge #98033

@joshtriplett
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Oct 9, 2022

⌛ Trying commit 7887b1be32b604959cb436e964c2698bfca3122c with merge c87f0b4449c495495344282b0457bd2cd7f66eb0...

@the8472
Copy link
Member

the8472 commented Oct 9, 2022

This seems like the wrong place. For other public, os-specific types the impl AsFd lives somewhere in os::<platform> modules, not in sys. There impls in the sys modules are for the internal types.

@bors
Copy link
Contributor

bors commented Oct 9, 2022

☀️ Try build successful - checks-actions
Build commit: c87f0b4449c495495344282b0457bd2cd7f66eb0 (c87f0b4449c495495344282b0457bd2cd7f66eb0)

@joshtriplett
Copy link
Member Author

This seems like the wrong place. For other public, os-specific types the impl AsFd lives somewhere in os::<platform> modules, not in sys. There impls in the sys modules are for the internal types.

impl AsFd for stdio types lives in library/std/src/sys/unix/stdio.rs on UNIX, as well.

@the8472
Copy link
Member

the8472 commented Oct 9, 2022

This is a bit inconsistent, e.g. AsRawFd comes from from os::fd::raw https://doc.rust-lang.org/src/std/os/fd/raw.rs.html#183-188
And the impl AsFd for File comes from os too. https://doc.rust-lang.org/src/std/os/fd/owned.rs.html#258-263

rust-lang#100892 implemented AsFd for the
sys versions, rather than for the public types. Change the
implementations to apply to the public types.
@joshtriplett joshtriplett force-pushed the bugfix-impl-fd-traits-for-io-types branch from 7887b1b to 88bb4e4 Compare October 9, 2022 18:02
@joshtriplett
Copy link
Member Author

@the8472 That would be a substantially larger reorganization, affecting multiple targets. No objections to such a reorganization, but that seems orthogonal to this bugfix.

That said, it does look like impl AsRawFd for io::Stdin would be redundant, so I'll remove that.

@the8472
Copy link
Member

the8472 commented Oct 9, 2022

Not suggesting to fix everything. Just put the Stdio ones in the right place.

@joshtriplett
Copy link
Member Author

Alright, let's hope this actually builds everywhere.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 11, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Oct 11, 2022

📌 Commit ef68327 has been approved by m-ou-se

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 11, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 11, 2022
…-for-io-types, r=m-ou-se

impl AsFd and AsRawFd for io::{Stdin, Stdout, Stderr}, not the sys versions

rust-lang#100892 implemented AsFd for the
sys versions, rather than for the public types. Change the
implementations to apply to the public types.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 11, 2022
…-for-io-types, r=m-ou-se

impl AsFd and AsRawFd for io::{Stdin, Stdout, Stderr}, not the sys versions

rust-lang#100892 implemented AsFd for the
sys versions, rather than for the public types. Change the
implementations to apply to the public types.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 12, 2022
…-for-io-types, r=m-ou-se

impl AsFd and AsRawFd for io::{Stdin, Stdout, Stderr}, not the sys versions

rust-lang#100892 implemented AsFd for the
sys versions, rather than for the public types. Change the
implementations to apply to the public types.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 14, 2022
…-for-io-types, r=m-ou-se

impl AsFd and AsRawFd for io::{Stdin, Stdout, Stderr}, not the sys versions

rust-lang#100892 implemented AsFd for the
sys versions, rather than for the public types. Change the
implementations to apply to the public types.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2022
Rollup of 8 pull requests

Successful merges:

 - rust-lang#102847 (impl AsFd and AsRawFd for io::{Stdin, Stdout, Stderr}, not the sys versions)
 - rust-lang#102856 (Only test duplicate inherent impl items in a single place)
 - rust-lang#102914 (Migrate css highlight without change)
 - rust-lang#102938 (Move some tests to more reasonable directories)
 - rust-lang#103015 (fix a typo)
 - rust-lang#103018 (More dupe word typos)
 - rust-lang#103025 (rustdoc: remove unused CSS `.search-container > *`)
 - rust-lang#103031 (Suppress irrefutable let patterns lint for prefixes in match guards)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
albertlarsan68 added a commit to albertlarsan68/rust that referenced this pull request Oct 14, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#102623 (translation: eager translation)
 - rust-lang#102769 (Clean up rustdoc startup)
 - rust-lang#102830 (Unify `tcx.constness` query and param env constness checks)
 - rust-lang#102847 (impl AsFd and AsRawFd for io::{Stdin, Stdout, Stderr}, not the sys versions)
 - rust-lang#102883 (Fix stabilization of `feature(half_open_range_patterns)`)
 - rust-lang#102936 (rustdoc: remove unused CSS `nav.sum`)
 - rust-lang#102940 (Update books)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
albertlarsan68 added a commit to albertlarsan68/rust that referenced this pull request Oct 14, 2022
Rollup of 8 pull requests

Successful merges:

 - rust-lang#102847 (impl AsFd and AsRawFd for io::{Stdin, Stdout, Stderr}, not the sys versions)
 - rust-lang#102856 (Only test duplicate inherent impl items in a single place)
 - rust-lang#102914 (Migrate css highlight without change)
 - rust-lang#102938 (Move some tests to more reasonable directories)
 - rust-lang#103015 (fix a typo)
 - rust-lang#103018 (More dupe word typos)
 - rust-lang#103025 (rustdoc: remove unused CSS `.search-container > *`)
 - rust-lang#103031 (Suppress irrefutable let patterns lint for prefixes in match guards)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b03bece into rust-lang:master Oct 14, 2022
@rustbot rustbot added this to the 1.66.0 milestone Oct 14, 2022
@joshtriplett joshtriplett deleted the bugfix-impl-fd-traits-for-io-types branch October 14, 2022 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants