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.

Sign up for GitHub

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

Jump to bottom

Remove unused Maybe wrapper around raw standard streams #75772

Merged
merged 3 commits into from
Aug 22, 2020
Merged

Remove unused Maybe wrapper around raw standard streams #75772

merged 3 commits into from
Aug 22, 2020

Conversation

tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Aug 21, 2020

  • Remove result type from raw standard streams constructors
  • Make raw standard stream constructors const
  • Remove wrapper type handling absent raw standard streams

cargo checked with:

env CC=true ./x.py check library/std/ \
  --target i686-unknown-linux-gnu \
  --target wasm32-unknown-emscripten \
  --target wasm32-wasi \
  --target x86_64-fortanix-unknown-sgx \
  --target x86_64-pc-windows-gnu \
  --target x86_64-unknown-cloudabi \
  --target x86_64-unknown-hermit \
  --target x86_64-unknown-linux-gnu \
  --target x86_64-uwp-windows-gnu \
  --target x86_64-wrs-vxworks

Note: Last target doesn't compile currently.

tmiasko added 3 commits August 21, 2020 13:17
@tmiasko
Remove result type from raw standard streams constructors
479c23b 
Raw standard streams constructors are infallible. Remove unnecessary
result type.
@tmiasko
Make raw standard stream constructors const
4a00421
@tmiasko
Remove wrapper type handling absent raw standard streams
78e0946 
Raw standard streams are always available.  Remove unused wrapper type
that was supposed to be responsible for handling their absence.
@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 Aug 21, 2020
dtolnay
dtolnay requested changes Aug 21, 2020
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Thanks, this is great. Two minor comments below.

library/std/src/io/stdio.rs
@@ -50,8 +50,9 @@ struct StderrRaw(stdio::Stderr);
/// handles is **not** available to raw handles returned from this function.
///
/// The returned handle has no external synchronization or buffering.
fn stdin_raw() -> io::Result<StdinRaw> {
stdio::Stdin::new().map(StdinRaw)
#[unstable(feature = "libstd_sys_internals", issue = "none")]
Copy link
Member

Choose a reason for hiding this comment

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

All these stability attributes should be unnecessary, since the functions are private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The windows stdio implementation has an unstable attribute. The stable /
unstable boundary cannot be crossed within const functions (although I am not
sure about exact rules there), so I had to marked those as unstable.

Copy link
Member

Choose a reason for hiding this comment

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

Ah that's too bad. I filed #75794 to follow up separately.

library/std/src/io/stdio.rs
self.0.read_to_string(buf)
}

fn read_exact(&mut self, buf: &mut [u8]) -> io::Result<()> {
Copy link
Member

Choose a reason for hiding this comment

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

Why is read_exact removed 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.

It is convenient to use default implementation, which does the right thing
here. In contrast to say handle_ebadf which wouldn't.

I can delegate it directly to the inner reader if you prefer.

@dtolnay dtolnay mentioned this pull request Aug 21, 2020
Closed
@dtolnay dtolnay added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 22, 2020
@dtolnay
Copy link
Member

dtolnay commented Aug 22, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Aug 22, 2020

📌 Commit 78e0946 has been approved by dtolnay

@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 Aug 22, 2020
@bors
Copy link
Contributor

bors commented Aug 22, 2020

⌛ Testing commit 78e0946 with merge ebc03f7...

@bors
Copy link
Contributor

bors commented Aug 22, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: dtolnay
Pushing ebc03f7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 22, 2020
@bors bors merged commit ebc03f7 into rust-lang:master Aug 22, 2020
@tmiasko tmiasko deleted the io-maybe-no branch August 22, 2020 07:32
@rust-log-analyzer
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
##[section]Starting: Request a runner to run this job
Found online and idle self-hosted runner in current repository that matches the required labels: 'self-hosted , ARM64 , linux'

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
merged-by-bors This PR was explicitly merged by bors. 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
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.
6 participants
@tmiasko @rust-highfive @dtolnay @bors @rust-log-analyzer @cuviper