Add missed unix filetypes (block/char devices, fifos and sockets) #26766
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Seeing the blame of the libstd/fs.rs file, looks like the correct reviewer could be @alexcrichton I'm sorry, I read the contribution instructions after the PR. |
|
|
||
| /// Test whether this file type represents a block device. | ||
| #[unstable(feature = "more_filetypes", reason = "recently added API")] | ||
| pub fn is_block_device(&self) -> bool { self.0.is_block_device() } |
There was a problem hiding this comment.
more_filetypes sounds very general. Do you think file_types_posix is a better name? (Just to suggest something too rather than just nitpicking!)
There was a problem hiding this comment.
completely agree, I like your name proposal, I'll make a commit to change it. Thanks! 😄
|
Edited: Aha, so it doesn't work if I say it, but that above it what you would say to have rust highfive reassign the issue. |
| #[unstable(feature = "file_types_posix", reason = "recently added API")] | ||
| pub fn is_block_device(&self) -> bool { false } | ||
| #[unstable(feature = "file_types_posix", reason = "recently added API")] | ||
| pub fn is_char_device(&self) -> bool { false } |
There was a problem hiding this comment.
This isn't obviously correct; Windows has device files. For example, NUL.
There was a problem hiding this comment.
Sorry, my knowledge about windows is really límited, i going to investigate it and try to propose a solution. Thanks!! 😄
|
I've added a proposal for the windows device file type, but I'm not sure if this is the correct appoach, because my windows knowledge is very limited. I don't know if the windows devices are char devices or block devices, then I have written it to returns true for is_char_device and for is_block_device when is a windows device (this may be wrong, anybody with knowledge about windows that can review this?) |
|
I've to define the S_IFSOCK in the liblibc/lib.rs for the different architectures. I'll do it today or tomorrow, I have to investigate the value of the that constants for the different architectures. |
|
Thanks for the PR! I would expect these functions to appear on a unix-specific extension trait rather than the |
|
Completely agree, I going to rewrite all this as a trait. Thanks! |
jespino
force-pushed
the
add-more-filetypes
branch
2 times, most recently
from
f724501 to
8ede8ee
Compare
|
I have reset the branch and write again the code. Now is done through a trait. |
| #[unstable(feature = "file_types_posix", reason = "recently added API")] | ||
| pub trait FileTypeUnix { | ||
| /// Add special unix types (block/char device, fifo and socket) | ||
| #[unstable(feature = "file_types_posix", reason = "recently added API")] |
There was a problem hiding this comment.
You should actually be able to omit these unstable tags because #[unstable] inherits (from the trait)
|
Now I thinks is ready and working. @alexcrichton If you want, I can rebase it to master and compact in one commit after your reveiw 😄 |
| #[unstable(feature = "file_type_ext", reason = "recently added API")] | ||
| impl FileTypeExt for ::fs::FileType { | ||
| fn raw_mode(&self) -> mode_t { self.as_inner().mode } | ||
| } |
There was a problem hiding this comment.
You can probably actually avoid this trait impl entirely and instead just making the is method below public and updating the impl in ext/fs.rs to do something like:
pub fn is_foo(&self) -> bool { self.as_inner().is(libc::S_IFFOO) }
|
Changed! 😄 |
|
Looks good to me, thanks @jespino! Flagging as T-libs and I-needs-decision so this comes up during triage. |
alexcrichton
added
I-needs-decision
|
Thanks! Now you have reviewed it, you prefer I leave the commits as are now, or you prefer I rebase it from master and compact everything in one commit? |
|
It's fine to leave as-is, I think there may be a few more minor updates that come through (e.g. docs) so the squashing can happen later. |
|
Ok we discussed this at triage today and the decision was to go ahead and just r+, I'll leave a few more comments and then I think this is good to go after a rebase. |
| @@ -178,6 +179,23 @@ impl MetadataExt for fs::Metadata { | |||
| } | |||
| } | |||
|
|
|||
| #[unstable(feature = "file_type_ext", reason = "recently added API")] | |||
| pub trait FileTypeExt { | |||
| /// Add special unix types (block/char device, fifo and socket) | |||
There was a problem hiding this comment.
This comment I believe should be placed on the trait itself. Also could you add documentation for each of these methods to say what it's doing?
jespino
force-pushed
the
add-more-filetypes
branch
from
5d8210f to
74f4298
Compare
|
Rebased to master, compacted in one commit, and rewritten the doc comments. |
I find that isn't supported on the current API and I think is necesary. It is my first PR to rust (I'm not a rust expert and I'm not sure if this is the better way to propose this thinks), of course any suggestion of change will be welcome. I'm almost sure that in windows aren't supported this filetypes, then, i put in the api of win::fs the functions with a fixed false in the response, I hope this is correct.
I find that isn't supported on the current API and I think is necesary.
It is my first PR to rust (I'm not a rust expert and I'm not sure if this is the better way to propose this thinks), of course any suggestion of change will be welcome.
I'm almost sure that in windows aren't supported this filetypes, then, i put in the api of win::fs the functions with a fixed false in the response, I hope this is correct.