-
Notifications
You must be signed in to change notification settings - Fork 13k
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
std: Stabilize a number of new fs features #25844
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I probably missed the discussion but why is this not an (extensible) enum? This seems like a really good candidate. Yes, some of the variants won't make sense on certain platforms but I don't think that's really an issue (we'll just have extra variants) and being able to
match
on filetype would be really nice.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variants:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're currently not necessarily considering a hidden/unstable variant to be an "unstable enum", it was mostly just how
ErrorKind
worked out. The set of file types also varies quite a bit across platforms, and the extra types made somewhat more sense to provide through extension traits instead of via thisenum
.Finally, using an opaque structure allows one day accessing the raw bits used to determine the file type which may contain some unknown information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually makes it easier to write cross-platform code because the coder doesn't need to care about the platform: they just handle all cases on all platforms (when possible).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's somewhat of a fine line between being able to write cross-platform code while still understand what is indeed cross platform. If we were to have an exhaustive enum which is the union of all file types on all platforms, it's suddenly not clear what file types come up on which platforms. Which is to say that it's not clear to me that having a union is indeed more cross platform as you're probably still going to have platform-specific code handling various file types on various platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we're already doing this, I don't see why we shouldn't do it here.
The problem with extension traits is that they force the user to care about the platform. With an enum, I could write:
With extension traits, I need one function per platform (each one importing a different extension trait). Extension traits are useful when some set of functionality is nonsensical on a platform but in this case, the extra file types make sense, they just aren't used.
This unknown information is stored in the Metadata structure. Are you worried about unknown file types or something? Pretty much by definition, a file should only have one type so an enum should always be able to describe it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm not re-loading before posting... For now, I feel that the enum is small enough that this isn't really an issue however, I can see this becoming an issue in the future as more platforms are added and this enum grows. So I see your point.