dirfd: initial quick and dirty implementation for unix by Qelxiros · Pull Request #139514 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Tracking issue: #120426
As per this comment, this issue needs someone to start work on an implementation, so I've implemented a couple functions for UNIX. There's a lot more work to be done here (most of the feature), so I'd love some guidance on what needs fixing in this PR and any notes on how to proceed. Thanks!
rustbot has assigned @workingjubilee.
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
Status: Awaiting review from the assignee but also interested parties.
Relevant to the library team, which will review and decide on the PR/issue.
labels
This comment has been minimized.
| @@ -1353,6 +1360,25 @@ impl Seek for Arc { |
|---|
| } |
| } |
| #[unstable(feature = "dirfd", issue = "120426")] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes on each of the functions, not the impl block
Comment on lines 1365 to 1445
| /// Opens a file relative to this directory. |
|---|
| pub fn open<P: AsRef |
| self.inner.open(path).map(|f |
| } |
| /// Opens a file relative to this directory with the specified options. |
| pub fn open_with<P: AsRef |
| self.inner.open_with(path, &opts.0).map(|f |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add examples here, as well as tests? Tests can go in std/src/fs/tests.rs.
| @@ -116,6 +116,13 @@ pub struct File { |
|---|
| inner: fs_imp::File, |
| } |
| #[unstable(feature = "dirfd", issue = "120426")] |
| #[cfg(target_family = "unix")] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't/can't have platform-gated config in fs, tidy would probably complain about this if CI got that far. It's okay to stub out less popular platforms for now with FIXMEs, but I think the initial implementation should come with Windows support. There are some guidelines at #120426 (comment), feel free to ask questions on Zulip if you get stuck.
Comment on lines 301 to 306
| // pub fn create_dir<P: AsRef |
| // pub fn rename<P: AsRef |
| // pub fn remove_file<P: AsRef |
| // pub fn remove_dir<P: AsRef |
| // pub fn symlink<P: AsRef |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably better to delete this, instead of possibly forgetting to remove them later.
| @@ -116,6 +116,13 @@ pub struct File { |
|---|
| inner: fs_imp::File, |
| } |
| #[unstable(feature = "dirfd", issue = "120426")] |
| #[cfg(target_family = "unix")] |
| /// An object providing access to a directory on the filesystem. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate the documentation here a bit? At least make it clear that this is a handle, and what happens on drop (similar to File).
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
| // dirfd isn't supported everywhere |
| #[cfg(not(any( |
| miri, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we handle such missing platforms by returning errors instead of not having the methods exist at all
This comment has been minimized.
This comment has been minimized.
The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)
Labels
Status: This is awaiting some action (such as code changes or more information) from the author.
Relevant to the library team, which will review and decide on the PR/issue.