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

Function to convert OpenOptions to c_int #76110

Merged
merged 8 commits into from
Sep 22, 2020
Merged

Function to convert OpenOptions to c_int #76110

merged 8 commits into from
Sep 22, 2020

Conversation

FedericoPonzi
Copy link
Contributor

Fixes: #74943
The creation_mode and access_mode function were already available in the OpenOptions struct, but currently private. I've added a new free functions to unix/fs.rs which takes the OpenOptions, and returns the c_int to be used as parameter for the open call.

@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 Aug 30, 2020
@FedericoPonzi
Copy link
Contributor Author

r? @withoutboats

@FedericoPonzi
Copy link
Contributor Author

I would like some guidance on the following:

  1. Is that the right place to put the requested function?
  2. How can I fix the CI error? Currently failing with error: function is never used: get_openopetions_as_cint`.
  3. Were can I put a test for it?

@FedericoPonzi @pickfire
Fix typo get openoptions function name
eb3906b 
Co-authored-by: Ivan Tham <pickfire@riseup.net>
joshtriplett
joshtriplett requested changes Aug 30, 2020
View reviewed changes
Copy link
Member

@joshtriplett joshtriplett left a comment

Choose a reason for hiding this comment

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

I added a couple of comments.

Also, you should add tests near the existing tests for OpenOptionsExt.

library/std/src/sys/unix/fs.rs Outdated Show resolved Hide resolved
library/std/src/sys/unix/fs.rs Outdated Show resolved Hide resolved
@FedericoPonzi
Copy link
Contributor Author

I added a couple of comments.

Also, you should add tests near the existing tests for OpenOptionsExt.

Thanks a lot for your comments! I've added the as_flags method to OpenOptionsExt in unix/ext/fs.rs. Since the get_access_mode and get_creation_mode are private to the OpenOptions struct thus I had to duplicate a bit of the code.

I currently have one last issue, I cannot seem to find the location of the tests for OpenOptionsExt, could you please tell me where can I find them?

@joshtriplett
Copy link
Member

joshtriplett commented Aug 30, 2020 •
edited
Loading

Since the get_access_mode and get_creation_mode are private to the OpenOptions struct thus I had to duplicate a bit of the code.

Similar to the other methods of OpenOptionsExt, you should implement this as a call to self.as_inne().as_flags(), which calls the corresponding method on the OpenOptions implemented in at library/std/src/sys/unix/fs.rs. That's where get_access_mode and get_creation_mode are defined, so you'll be able to call them and avoid the duplication.

I currently have one last issue, I cannot seem to find the location of the tests for OpenOptionsExt, could you please tell me where can I find them?

I'd suggest adding doctests to the method, like the other methods of OpenOptionsExt have.

@FedericoPonzi
Copy link
Contributor Author

FedericoPonzi commented Sep 7, 2020 •
edited
Loading

r? @joshtriplett
I've r you because you already approved the PR, and this hadn't any traffic for a while now and I'm not sure how to move this forward. Hopefully it's fine for you, thanks!

@jyn514 jyn514 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 15, 2020
@FedericoPonzi FedericoPonzi mentioned this pull request Sep 16, 2020
Open
2 tasks
@jyn514
Copy link
Member

jyn514 commented Sep 22, 2020 •
edited
Loading

@bors r=@joshtriplett

@bors
Copy link
Contributor

bors commented Sep 22, 2020

📌 Commit 2f51922 has been approved by JoshTriplet

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

bors commented Sep 22, 2020

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Sep 22, 2020

📌 Commit 2f51922 has been approved by JoshTriplett

@bors
Copy link
Contributor

bors commented Sep 22, 2020

⌛ Testing commit 2f51922 with merge e0bc267...

@bors
Copy link
Contributor

bors commented Sep 22, 2020

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 22, 2020
@bors bors merged commit e0bc267 into rust-lang:master Sep 22, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 22, 2020
@FedericoPonzi FedericoPonzi deleted the convert-openoptions-cint branch September 22, 2020 15:30
@dtolnay dtolnay mentioned this pull request Sep 23, 2020
Closed
@jyn514 jyn514 mentioned this pull request Sep 23, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2020
@bors
Auto merge of rust-lang#77090 - rust-lang:revert-76110-convert-openop…
a6008fa 
…tions-cint, r=dtolnay

Revert "Function to convert OpenOptions to c_int"

Reverts rust-lang#76110. This broke Rust's stability guarantees.

Closes rust-lang#77089.

r? `@joshtriplett`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

@pickfire pickfire pickfire left review comments

@joshtriplett joshtriplett joshtriplett approved these changes

Assignees

@joshtriplett joshtriplett

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
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.

Support for converting OpenOptions to c_int on UNIX
9 participants
@FedericoPonzi @rust-highfive @joshtriplett @jyn514 @bors @pickfire @Mark-Simulacrum @withoutboats @rustbot