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

WIP: Document From implementations in std::sys::unix::process #96313

Closed

Conversation

LordRatte
Copy link

Document From implementations for Stdio, ExitCode

In connection with #51430

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

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 r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. 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

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 22, 2022
@LordRatte
Copy link
Author

Hi, @skade

I am still unsure about ExitCode's implementation. I want to say that it doesn't copy because it uses a u8.

Would that be correct?

/// Converts a [`AnonPipe`] into a [`Stdio`].
///
/// The conversion consumes the [`AnonPipe`].
///
fn from(pipe: AnonPipe) -> Stdio {
Stdio::Fd(pipe.into_inner())
}
}

impl From<File> for Stdio {
Copy link
Member

Choose a reason for hiding this comment

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

These two implementations are not exposed outside of std, as Stdio here is an internal type (defined in this very module, and not public). This can be a little confusing since there's also a public std::process::Stdio, but that's an unrelated type.

I don't think it's useful to add documentation to these, particularly since these impls are pretty trivial -- we're not doing anything interesting in them.

@Mark-Simulacrum
Copy link
Member

I am still unsure about ExitCode's implementation. I want to say that it doesn't copy because it uses a u8.

It looks like that link doesn't really work for me, but presuming you mean this implementation, I don't think that's something we need to add a doc comment to either. It's another internal implementation detail of std, since the ExitCode in question is an internal type. The actual From impl already has a short documentation comment, and there's more detail provided on the std::process::ExitCode type itself.

I'm going to close this PR since I don't think we need to document these two particular From impls. In general, I'm not sure how much value documentation on From impls serves when they are just performing the obvious conversion between the two types, unless we have more detail to offer (e.g., the potential reallocation made in From<Vec<T> for Box<[T]>). I think most of that ground has been covered, and is hard to search for automatically. Thanks for proposing this though!

You can look at some other issues we've tagged as E-easy, which can be good starting points to contributing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

5 participants