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.
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
Implement
FileDescriptor.Pipe()
#58Implement
FileDescriptor.Pipe()
#58Changes from 5 commits
fd1e85e
e0d8759
6a99c59
c51b1d1
af3ab71
534e55f
18b66f9
1b37611
b7bb636
64a60aa
a210eda
eec2311
48d4d94
3c85094
beaeff2
e80e39c
86ad6f9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
You might be able to say
var fds: Array<CInt> = [-1, -1]
and use the implicit array-to-pointer conversion by passing&fds
to the syscall (though I don't recall if it still works or has limitations). Otherwise, you can probably usewithUnsafeMutablePointer
to avoid the extra bind-memory step.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.
@lorentey suggested I use the tuple form for a stronger guarantee that those values would end up on the stack. I haven't checked but I believe
withUnsafeMutablePointer
will still require rebinding the type (I think this was a signedness issue), but would also require manually passing the count (1), whereaswithUnsafeMutableBytes
takes this information from the buffer pointer.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.
Please do make sure that this builds on windows - I'm almost certain that this will break the Windows builds. Also note that Windows should transact in
HANDLE
s, akavoid *
, so this is going to truncate at least. SeeCreatePipe
.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 fine to simply surround these additions with
#if !os(Windows)
for now.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 looking great! @atrick / @glessard , what's the lesser of evils for the pointer binding stuff?
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.
The pointer to a homogenous tuple is also bound to its element type, so you can use
assumingMemoryBound
.(I hope. I use this quite a lot)
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.
As written, this case indeed calls for
assumingMemoryBound
.The memory is already bound to
Int32
, sobindMemory
doesn't add information.This being said, we unfortunately don't have
assumingMemoryBound
on theRawBufferPointer
types at this point. We also gain nothing from using a Buffer in this case: we must rely in the C call to be well behaved instead of having any sort of bounds checking. Given that, I suggest this for lines 389-393: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.
@karl is correct, and @glessard 's code edit looks good.
Although the original code was harmless, it doesn't really make sense to bind memory that belongs to a variable.
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.
Is there a document detailing what "binding memory" actually does? I had thought this was just managing compile-time information so I'm not 100% clear on the difference between "assuming" memory bound and actually binding memory.
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.
There is some scattered documentation, including the doc-comments, but the largest chunk is in the
RawPointer
proposal: https://github.com/apple/swift-evolution/blob/main/proposals/0107-unsaferawpointer.mdExperience has shown that those sources are insufficient.
@NevinBR (I think) came up with a nice description of binding for humans a few weeks ago: https://forums.swift.org/t/pitch-implicit-pointer-conversion-for-c-interoperability/51129/36.
In general, if you're reminding the compiler of type information it should already have known, but had been obscured for some reason, then
assumingMemoryBound
is the thing to reach for (as we did here). If you are telling the compiler new information, then reach forbindMemory
orwithMemoryRebound
.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 probably do (not gonna hold up this PR for it though) want to allow for mock testing of such stack local pointers. We'd probably have a variant that we'd pass in an array and it would compare the contents.