FileOperations: Add Windows implementation for FileDescriptor.resize(to:) #89
Conversation
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
…to:) Signed-off-by: Si Beaumont <beaumont@apple.com>
simonjbeaumont
commented
Jun 6, 2022
Comment on lines
108
to
114
| @inline(__always) | ||
| internal func ftruncate( | ||
| _ fd: Int32, | ||
| _ length: off_t | ||
| ) -> Int32 { | ||
| _chsize(fd, numericCast(length)) | ||
| } |
There was a problem hiding this comment.
@compnerd do you have any opinions on this? Do you think that _chsize is the right choice for Windows or should we be using implementing this in terms of other APIs? You mentioned something about SetFilePointerEx and SetEndOfFile?
There was a problem hiding this comment.
In general, using the windows API is better. It tends to be more flexible and integrates better by covering more cases (e.g. file descriptors are really a lookup table in the C library to the underlying kernel handle, and that may be created bypassing the registration by using the Windows API).
At the very least please use _chsize_s (which in fact I have a pending patch for NIO) as that uses a __int64 for the offset rather than a long which is 32-bit. It effectively is the behavioral change of _FILE_OFFSET_BITS=64 on GNU. I don’t see a reason for modern code to use the 32-bit offset even under register pressure on 32-bit platforms in a library.
For keeping the signature the same, we can use _get_osf_handle to get the kernel handle from the file descriptor (as +0! do not free, aka CloseHandle). We should expose the handle variant of the API, but that can be done subsequently.
There was a problem hiding this comment.
I do have some updated docs at https://github.com/compnerd/swift-build/blob/master/docs/WindowsQuickStart.md. I’m working on some more options for building.
Signed-off-by: Si Beaumont <beaumont@apple.com>
|
@swift-ci please test |
2 tasks
compnerd
approved these changes
Jun 7, 2022
|
@swift-ci please test |
|
Hmm, seems that this actually didn't build :-(: |
I think there's this guard that needs removing... https://github.com/apple/swift-system/blob/main/Sources/System/Internals/Syscalls.swift#L128 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
#82 added
FileDescriptor.resize(to:). The original PR included addingftruncatetoWindowsSycallAdapters.swiftbut it was removed during review, and so there is currently no implementation forresize(to:)on Windows.The justification was that we didn't want to shoehorn Windows support into the POSIX code paths and that the Windows SDK was different enough to justify its own platform-specific implementation. However, looking at
chsize(renamed_chsize) it looks like it's supposed to be the equivalent to the POSIX-like functions in UCRT.In order to move discussion forward on an appropriate Windows implementation I have reinstated the syscall adapter as a strawman proposal.