-
Notifications
You must be signed in to change notification settings - Fork 113
FileDescriptor: Add resize(to newSize:) and fileSize() APIs #82
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
Conversation
Signed-off-by: Si Beaumont <[email protected]>
@inline(__always) | ||
internal func ftruncate( | ||
_ fd: Int32, _ length: off_t | ||
) -> Int32 { | ||
_chsize(fd, numericCast(length)) | ||
} |
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.
Full disclosure: I have not tested the Windows path. AFAICT there isn't a Windows CI pipeline?
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 believe it's best to surround the addition of resize
with #if !os(Windows)
.
Generally, I don't think it's a good idea to try to shoehorn Windows support into POSIX code paths here, or anywhere else in this code base. Swift System is not intended to provide a portable API layer, and the Windows SDK is different enough that it deserves its own, standalone, implementations.
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.
OK, apologies for that, I was just following the pattern I saw with the existing WindowsSyscallAdapters
. I added some fixups which move this API to only non-Windows platforms:
@swift-ci please test |
Signed-off-by: Si Beaumont <[email protected]>
@swift-ci please test |
@lorentey can you take an initial look? |
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.
Big thumbs up on resize
! I'm less convinced we need fileSize
.
Let's keep the Windows implementation out for now.
Sources/System/FileHelpers.swift
Outdated
) throws -> Int64 { | ||
let current = try seek(offset: 0, from: .current) | ||
let size = try seek(offset: 0, from: .end) | ||
try seek(offset: current, from: .start) |
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 is temporarily mutating the file position during what looks like a read-only operation; I feel the potential pitfalls of this (alongside the fact that many file descriptors do not have a file size) probably makes this entry point less suitable for this package.
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.
That's reasonable. I added the following fixup to address this:
- d90bf85
fixup: Move FileDescriptor.fileSize() to an internal extension in SystemTests
@inline(__always) | ||
internal func ftruncate( | ||
_ fd: Int32, _ length: off_t | ||
) -> Int32 { | ||
_chsize(fd, numericCast(length)) | ||
} |
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 believe it's best to surround the addition of resize
with #if !os(Windows)
.
Generally, I don't think it's a good idea to try to shoehorn Windows support into POSIX code paths here, or anywhere else in this code base. Swift System is not intended to provide a portable API layer, and the Windows SDK is different enough that it deserves its own, standalone, implementations.
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.
Thanks @lorentey for the review!
Big thumbs up on
resize
! I'm less convinced we needfileSize
.
No problem, I moved fileSize
to be an implementation detail of the tests for resize
.
Let's keep the Windows implementation out for now.
Done.
I also updated the PR description to reflect the changes.
Sources/System/FileHelpers.swift
Outdated
) throws -> Int64 { | ||
let current = try seek(offset: 0, from: .current) | ||
let size = try seek(offset: 0, from: .end) | ||
try seek(offset: current, from: .start) |
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.
That's reasonable. I added the following fixup to address this:
- d90bf85
fixup: Move FileDescriptor.fileSize() to an internal extension in SystemTests
@inline(__always) | ||
internal func ftruncate( | ||
_ fd: Int32, _ length: off_t | ||
) -> Int32 { | ||
_chsize(fd, numericCast(length)) | ||
} |
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.
OK, apologies for that, I was just following the pattern I saw with the existing WindowsSyscallAdapters
. I added some fixups which move this API to only non-Windows 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.
LGTM
@swift-ci please test |
Signed-off-by: Si Beaumont <[email protected]>
@swift-ci please test |
Signed-off-by: Si Beaumont <[email protected]>
@swift-ci please test |
…r ExpressibleByDictionaryLiteral (apple#82)
…r ExpressibleByDictionaryLiteral (apple#82) # Conflicts: # Sources/PriorityQueueModule/Heap+ExpressibleByArrayLiteral.swift
The
FileDescriptor
API currently doesn't expose anything that resemblesftruncate(2)
for truncating or extending the file associated with a file descriptor.This PR adds a new API
FileDescriptor.resize(to newSize:)
which wrapsftruncate
, available on non-Windows platforms.