[Swift language features] prototype for SwiftOptional#2896
Merged
stephen-hawley merged 3 commits intofeature/swift-bindingsfrom Jan 6, 2025
Merged
[Swift language features] prototype for SwiftOptional#2896stephen-hawley merged 3 commits intofeature/swift-bindingsfrom
stephen-hawley merged 3 commits intofeature/swift-bindingsfrom
Conversation
jkurdek
reviewed
Dec 20, 2024
| /// <summary> | ||
| /// Marshals this object to a Swift destination | ||
| /// </summary> | ||
| unsafe IntPtr MarshalToSwift(IntPtr swiftDest); |
| /// <summary> | ||
| /// Creates a new Swift object from a given payload | ||
| /// </summary> | ||
| public static abstract ISwiftObject NewFromPayload(IntPtr payload); |
There was a problem hiding this comment.
I agree with you that SwiftHandle would feel better
|
|
||
| namespace Swift.Runtime; | ||
|
|
||
| #nullable enable |
There was a problem hiding this comment.
Maybe a broader discussion, should we enable nullable on all Swft.Runtime code?
Member
There was a problem hiding this comment.
Agreed but definitely not in this PR
| public bool HasValue => Case == SwiftOptionalCases.Some; | ||
| } | ||
|
|
||
| internal static class PInvokesForSwiftOptional { |
There was a problem hiding this comment.
Suggested change
| internal static class PInvokesForSwiftOptional { | |
| internal static class PInvokesForSwiftOptional { |
| /// <summary> | ||
| /// Represents a class for marshaling data to and from Swift | ||
| /// </summary> | ||
| public static class SwiftMarshal { |
There was a problem hiding this comment.
NIT: Sometimes a new line is used before a curly brace and sometimes not
|
|
||
| internal static class PInvokesForSwiftOptional { | ||
| [DllImport(KnownLibraries.SwiftCore, EntryPoint = "$sSqMa")] | ||
| public static extern TypeMetadata _MetadataAccessor(MetadataRequest request, TypeMetadata typeMetadata); |
There was a problem hiding this comment.
NIT: I'm not sure about the naming convention. In the generated code I think we use the "PInvoke" prefix. I dont have strong opinion what convention should we use, just want to be consistent.
dalexsoto
approved these changes
Dec 31, 2024
This was referenced Jan 9, 2025
This file contains hidden or 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
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.
This is a prototype of SwiftOptional. It's meant to flesh out issues that we will encounter in binding enums and binding generic types.
What to expect here:
What this is missing:
ISwiftEquatable and ISwiftHashable implementations for SwiftOptional. Mostly because those interface definitions and the necessary proxies aren't here (yet).
No memory management. This type should implement
IDisposable.What I like:
What I don't like:
SwiftObjectHelper<SwiftOptional<T>>.GetTypeMetadata()to get the metadata within a class, but I understand that it's the cost of the explicit interface and static methods and keeping only one copy of the type metadata.SwiftOptional<T>. This is an important point for discussion. What we need is a constructor that has a unique signature that is unpronounceable in Swift so that there will never be conflicts in the bindings. In BTfS I used an enum that was unique to BTfS. We could make the ctor do all the marshaling and take a SwiftHandle (which also shouldn't be pronounceable in Swift).Why MarshalToSwift returns a different pointer:
For generic arguments to functions, Swift uses a pointer to value types. For heap allocated types, the actual pointer is used NOT a pointer to a pointer. Because of this, a binding to a class will have something that looks like this for the marshaling:
Other notes:
I added
HasValueandValueas those will feel better to C# programmers. In general for binding, we should consider binding types that are represented as classes as partial classes so we can add convenience methods to them that have access to the internals.The pattern I used for enums follows the documentation that I wrote for binding enums in that:
[TYPE]CasesNew[CASENAME]Case[CASENAME]Given C#'s lack of discriminated unions, this seemed reasonable.