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

Add a BorrowedFd::try_clone_to_owned and accompanying documentation #97178

Merged
merged 4 commits into from
Jun 15, 2022

Conversation

sunfishcode
Copy link
Member

Add a BorrowedFd::try_clone_to_owned, which returns a new OwnedFd sharing the underlying file description. And similar for BorrowedHandle and BorrowedSocket on WIndows.

This is similar to the existing OwnedFd::try_clone, but it's named differently to reflect that it doesn't return Result<Self, ...>. I'm open to suggestions for better names.

Also, extend the unix::io documentation to mention that dup is permitted on BorrowedFd.

This was originally requsted here. At the time I wasn't sure whether it was desirable, but it does have uses and it helps clarify the API. The documentation previously didn't rule out using dup on a BorrowedFd, but the API only offered convenient ways to do it from an OwnedFd. With this patch, the API allows one to do try_clone on any type where it's permitted.

@sunfishcode sunfishcode added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 19, 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

r? @Mark-Simulacrum

(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 May 19, 2022
@sunfishcode
Copy link
Member Author

As an example use case, BorrowedFd::try_clone_to_owned would be useful in code like this. In that case, it would eliminate the need for the crate to depend on libc entirely.

@Mark-Simulacrum
Copy link
Member

r? @joshtriplett for sign-off on the new API

impl BorrowedFd<'_> {
/// Creates a new `OwnedFd` instance that shares the same underlying file
/// description as the existing `BorrowedFd` instance.
#[cfg(not(target_arch = "wasm32"))]
Copy link
Member

Choose a reason for hiding this comment

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

Does WASI not have a dup function?

Copy link
Member Author

@sunfishcode sunfishcode Jun 15, 2022

Choose a reason for hiding this comment

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

It does not, today. Because of all the descriptor vs. description issues, the original WASI left dup out to give itself flexibility to figure out what it wanted to say. Today we now have a clear sense of how we want this to work, and I expect WASI will add a dup. When it does, we can remove this code from Rust.

@joshtriplett
Copy link
Member

👍 for adding this, modulo the question about WASI.

@bors d+

Go ahead and merge when ready, though you may want to wait to avoid conflict with the in-flight stabilization.

@bors
Copy link
Contributor

bors commented Jun 15, 2022

☔ The latest upstream changes (presumably #98131) made this pull request unmergeable. Please resolve the merge conflicts.

And `BorrowedHandle::try_clone_to_owned` and
`BorrowedSocket::try_clone_to_owned` on Windows.
On Unix, describe these in terms of the underlying "file description". On
Windows, describe them in terms of the underlying "object".
@rust-log-analyzer

This comment has been minimized.

@sunfishcode
Copy link
Member Author

I've answered the WASI question above, and fixed the merge conflict.

@bors r+

@bors
Copy link
Contributor

bors commented Jun 15, 2022

@sunfishcode: 🔑 Insufficient privileges: Not in reviewers

@sunfishcode
Copy link
Member Author

@bors r=sunfishcode

@bors
Copy link
Contributor

bors commented Jun 15, 2022

@sunfishcode: 🔑 Insufficient privileges: Not in reviewers

@bjorn3
Copy link
Member

bjorn3 commented Jun 15, 2022

@bors r=joshtriplett

@bors
Copy link
Contributor

bors commented Jun 15, 2022

📌 Commit ee49d65 has been approved by joshtriplett

@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 Jun 15, 2022
@bors
Copy link
Contributor

bors commented Jun 15, 2022

⌛ Testing commit ee49d65 with merge b31f9cc...

@bors
Copy link
Contributor

bors commented Jun 15, 2022

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing b31f9cc to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 15, 2022
@bors bors merged commit b31f9cc into rust-lang:master Jun 15, 2022
@rustbot rustbot added this to the 1.63.0 milestone Jun 15, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b31f9cc): comparison url.

Instruction count

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)
N/A N/A 0
Regressions 😿
(secondary)
0.2% 0.2% 1
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)
0.1% 0.1% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-2.8% -4.5% 3
All 😿🎉 (primary) 0.1% 0.1% 1

Cycles

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 🎉 relevant improvement found
mean1 max count2
Regressions 😿
(primary)
3.9% 3.9% 1
Regressions 😿
(secondary)
N/A N/A 0
Improvements 🎉
(primary)
N/A N/A 0
Improvements 🎉
(secondary)
-2.3% -2.3% 1
All 😿🎉 (primary) 3.9% 3.9% 1

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants