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
[Swift language feature] Implement Swift.Array support #2964
[Swift language feature] Implement Swift.Array support #2964
Changes from 8 commits
8d0cb36
257f810
b4f379f
9b30b70
b3405f1
8835519
77baf32
32c3f4d
ebe2ea0
35c02a0
7d34964
9469c1c
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.
General comment about current design of MarshalToSwift - this feels very prone to buffer overruns.
It's a virtual call, so the caller technically doesn't know what the callee will be precisely. But at the same time, the caller MUST now the size of the buffer to allocate for swiftDest. That feels wrong.
What is the scenario where we have a preexisting buffer and need to marshal to it? So far the callers I've seen all need to allocate before calling this method. And they have to get the size right, otherwise we'll get buffer overruns at runtime.
I think the minimum should be that we don't pass a raw pointer, but some representation of pointer + size. Or better yet, this method would be responsible for allocating the buffer and returning it.
I also find it weird that the method takes a destination buffer, but it may choose to return another buffer. What is the memory ownership? We should at least precisely document this in comments, but ideally redesign the API to make it "secure by default".
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.
Maybe we could use
MemoryManager<byte>
(our own implementation) which would handle the memory ownership. But it's also possible that having a simple struct of our own would be good enough.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 agree that we should review the
MarshalToSwift
design. I would not tackle this in this PR though.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.
Absolutely - created #2974 to track that.
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.
Added to #2851
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 will have to be able to obtain the descriptor for
array : Collection
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.
Should this just copy the memory? What about arrays of reference types?
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 would remove this constructor as it goes against the expected behavior on Swift side. Even if payload value is copied, it would point to the same instance.
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.
Can't be removed, it is used in the
ISwiftObject.NewFromPayload
. It should perform a shallow copy of theArrayBuffer
struct.Anyway, it doesn't have the same semantics as in Swift.
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, maybe this is a future problem once we start supporting reference types. We might need to call
InitializeWithCopy
hereThere 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.
Could you provide an example?
Added to #2930.
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 don't have anything specific in my mind yet. I was think about having an array of reference types. If we just make a shallow copy of the array the ref count of those references will not be updated.