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

Provide optional Read/Write methods for stdio #136769

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

thaliaarchi
Copy link
Contributor

@thaliaarchi thaliaarchi commented Feb 9, 2025

Override more of the default methods for io::Read and io::Write for stdio types, when efficient to do so, and deduplicate unsupported types.

Tracked in #136756.

@rustbot
Copy link
Collaborator

rustbot commented Feb 9, 2025

r? @Noratrieb

rustbot has assigned @Noratrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added O-hermit Operating System: Hermit O-SGX Target: SGX O-solid Operating System: SOLID O-wasi Operating system: Wasi, Webassembly System Interface 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. labels Feb 9, 2025
@rust-log-analyzer

This comment has been minimized.

@thaliaarchi
Copy link
Contributor Author

@joboet Looks like you wrote #136780 to have minimum conflicts to this. Thanks for that. I don't mind rebasing onto that, if yours is merged first.

@Noratrieb
Copy link
Member

r? joboet

@rustbot rustbot assigned joboet and unassigned Noratrieb Feb 12, 2025
@thaliaarchi thaliaarchi changed the title Provide optional Read/Write method for stdio Provide optional Read/Write methods for stdio Feb 13, 2025
@thaliaarchi thaliaarchi force-pushed the io-optional-methods/stdio branch 2 times, most recently from f51bb4e to 118bb74 Compare February 16, 2025 21:59
@thaliaarchi
Copy link
Contributor Author

I dropped a few commits from this PR:

@thaliaarchi thaliaarchi marked this pull request as draft February 19, 2025 04:37
@thaliaarchi
Copy link
Contributor Author

Since this touches a lot of targets, I'm going to split this into multiple PRs for more target-focused review, once #136780 goes through the merge queue.

@joboet
Copy link
Member

joboet commented Feb 19, 2025

Since this touches a lot of targets, I'm going to split this into multiple PRs for more target-focused review, once #136780 goes through the merge queue.

That'd be great, thank you! Especially the SGX stuff would be much simpler to review as a dedicated PR.

@thaliaarchi thaliaarchi force-pushed the io-optional-methods/stdio branch from 118bb74 to ade071b Compare February 21, 2025 05:27
@rustbot rustbot added O-unix Operating system: Unix-like O-wasm Target: WASM (WebAssembly), http://webassembly.org/ O-windows Operating system: Windows labels Feb 21, 2025
@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 25, 2025
@thaliaarchi
Copy link
Contributor Author

@joboet Would you mind rebasing #136780 onto master so it can be merged and we can move forward with this?

@thaliaarchi thaliaarchi force-pushed the io-optional-methods/stdio branch 2 times, most recently from 36b01f3 to 46167f7 Compare March 10, 2025 08:16
@thaliaarchi thaliaarchi marked this pull request as ready for review March 10, 2025 08:16
@thaliaarchi
Copy link
Contributor Author

Now that #136780 has merged, we're ready for review again!

I've moved anything platform-specific out of this PR. #137355 is for SGX, #138301 is for Hermit, and I will open one for Xous after this merges. Everything else that was split out of this PR has already been merged.

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 10, 2025
@thaliaarchi thaliaarchi force-pushed the io-optional-methods/stdio branch from 46167f7 to 70afdf2 Compare March 10, 2025 08:42
@thaliaarchi
Copy link
Contributor Author

That being said, I copy the optional I/O methods to std::sys::stdio::unsupported which I add to std::io::Empty in #137051, which means any feedback from there will apply here. But, since this hasn't been reviewed in a while or without all the more platform-specific parts, a review here would still be helpful.

@thaliaarchi
Copy link
Contributor Author

@rustbot label -O-hermit -O-SGX -O-solid -O-unix -O-wasi -O-wasm -O-windows

@rustbot rustbot removed O-hermit Operating System: Hermit O-SGX Target: SGX O-solid Operating System: SOLID O-unix Operating system: Unix-like O-wasi Operating system: Wasi, Webassembly System Interface O-wasm Target: WASM (WebAssembly), http://webassembly.org/ O-windows Operating system: Windows labels Mar 10, 2025
Copy link
Member

@joboet joboet left a comment

Choose a reason for hiding this comment

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

Just some cosmetic suggestions:

Copy link
Member

Choose a reason for hiding this comment

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

I think making Stderr an alias to Stdin in some of the other modules is a clever idea. Perhaps you could do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I can do that. I think the only downside to these aliases is that Stderr is then misnamed as Stdout in stack traces.

@joboet joboet added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2025
Match what `std::io::Empty` does, since it is very similar.
This seems to be the pattern for newer pal stdio types.
@thaliaarchi thaliaarchi force-pushed the io-optional-methods/stdio branch from 70afdf2 to 46e3e46 Compare March 11, 2025 21:39
@thaliaarchi
Copy link
Contributor Author

Thanks for the review! I've updated to address your feedback.

@rustbot blocked (on #137051)

@rustbot rustbot added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Blocked on something else such as an RFC or other implementation work. 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