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

Document the default for ExitStatus #110863

Closed
jyn514 opened this issue Apr 26, 2023 · 13 comments
Closed

Document the default for ExitStatus #110863

jyn514 opened this issue Apr 26, 2023 · 13 comments
Assignees
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@jyn514
Copy link
Member

jyn514 commented Apr 26, 2023

@scottmcm: I think it should at least say somewhere what that default is. Having it come from a derive(Default) on a c_int feels indirect to me as well, so perhaps it would make sense to define it manually in terms of ExitCode::SUCCESS instead, say -- that would also allow putting a doc-comment on the impl.

We should document that Default for ExitStatus is "process reported successful termination", ie the result of ExitCode::success().into(), and, implement it that way.

Originally posted by @ijackson in #106425 (comment)

@jyn514 jyn514 added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools labels Apr 26, 2023
@scottmcm scottmcm added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Apr 26, 2023
@john-h-k
Copy link
Contributor

Is this possible to currently do? Having checked out the linked issue it looks like it depends on impl Default for ExitStatus existing

@jyn514
Copy link
Member Author

jyn514 commented Apr 27, 2023

Right, it needs #106425 to merge first.

@jyn514 jyn514 added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Apr 27, 2023
@john-h-k
Copy link
Contributor

Alright 👍 . How does documenting work for a derived trait?

@jyn514
Copy link
Member Author

jyn514 commented Apr 27, 2023

You'd have to remove the derive and use manual impl Default blocks.

@ijackson
Copy link
Contributor

ijackson commented Apr 27, 2023 via email

@john-h-k
Copy link
Contributor

john-h-k commented Apr 27, 2023

Also just to check, It's ExitCode::SUCCESS.into() right? success() doesn't seem to be a method and doesn't seem to be being added by that issue (or should I add that method?)

Also, should I open a PR (that is on top of the existing PR to actually add the default), or wait until the other is merged? Done the changes locally already

@ijackson
Copy link
Contributor

ijackson commented Apr 27, 2023 via email

@u-na-gi
Copy link

u-na-gi commented Aug 22, 2023

@rustbot claim

@ijackson
Copy link
Contributor

Note #114593, which is a bit of a swamp. I think changing the docs can be done (at the cost of adding to the set of things which the docs claim to be true but which are false on those broken platforms) but changing the implementation ("and implement it that way") is blocked.

@john-h-k
Copy link
Contributor

@rustbot claim

I’ve already got a branch with this issue fixed on it - was waiting for the previous PR to merge and it now has. For some reason I forget to claim it though

@u-na-gi u-na-gi removed their assignment Aug 23, 2023
@bkaestner
Copy link
Contributor

Documented as of a741a5a within 1.73 via #114864 or am I mistaken?

@HerrCai0907
Copy link

@rustbot claim

@john-h-k
Copy link
Contributor

@rustbot claim

It appears this has already been done - i mentioned a few comments back but I've already completed this - i was going to claim it but im awaiting an update on whether its already been finished

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants