[Swift language features] Implement Foundation.Data wrapper for the CryptoKit in C#
#3000
Conversation
| { | ||
| fixed (void* _payloadPtr = &this) | ||
| { | ||
| metadata.ValueWitnessTable->InitializeWithCopy((void*)swiftDest, (void*)_payloadPtr, metadata); |
There was a problem hiding this comment.
This is a broader question (not just Data): In MarshalToSwift we call InitializeWithCopy, when copying the content to a native memory buffer. But in the NewFromPayload (and in the constructor) we make a payload copy with just "memcpy". The question is why is it OK to do a simple memory copy when bringing the data into managed but it's not OK to do it when creating a native memory copy? It feels inconsistent.
There was a problem hiding this comment.
We've discussed this offline and it seems that the existing copy constructor is not valid.
There was a problem hiding this comment.
I'm sorry I was not able to be there for the discussion. Do we have some kind of writeup which describes the outcomes? We'll probably need some kind of rules to follow in other projections as well.
There was a problem hiding this comment.
Yes, sorry for the vague response -- I was waiting for this PR to streamline the feedback: #3008
| _protocolConformanceSymbols = new Dictionary<Type, string> { | ||
| { typeof(ISwiftDataProtocol), "$s10Foundation4DataVAA0B8ProtocolAAMc" }, | ||
| { typeof(ISwiftContiguousBytes), "$s10Foundation4DataVAA15ContiguousBytesAAMc" }, | ||
| }; |
There was a problem hiding this comment.
Side note (definitely not this PR): This induces a cost to just touching the Data type - it's basically a startup cost. But there's no good reason for it - this is data which is hardcoded on the type. What if we encoded this in "code". Basically instead generating code which fills dictionary - generate a switch statement. For smallish number of cases the perf is probably better for switch anyway and there's 0 startup cost (with AOT it's true 0, with JIT it's as lazy as it can be).
There was a problem hiding this comment.
Good point, thanks. Added to #2990.
| { | ||
| private long _flags; | ||
| private IntPtr _object; | ||
|
|
||
| private static nuint _payloadSize = SwiftObjectHelper<Data>.GetTypeMetadata().Size; |
There was a problem hiding this comment.
Since I'm nit picking and posting unrelated comments: This is another "startup cost" - this leads to several things:
- It gets the metadata (PInvoke)
- caches it
- Gets the size (another PInvoke)
And all of this will happen the first time anything just touches the Data type. While in reality we only need this when the type is instantiated (or maybe even later on).
Additionally I just noticed that this doesn't seem to be used anywhere... ???
There was a problem hiding this comment.
We should only add this when SwiftIndirectResult is used, since it appears to be the only case. I've added this to. Added to #2990.
| { | ||
| private long _flags; | ||
| private IntPtr _object; | ||
|
|
||
| private static nuint _payloadSize = SwiftObjectHelper<Data>.GetTypeMetadata().Size; |
There was a problem hiding this comment.
Nit: dotnet/runtime naming convention is to prefix static fields with s_ - in case you would like to be consistent with it.
There was a problem hiding this comment.
Thanks, we will update the generated and manual bindings.
Description
This PR improves the implementation of the
Foundation.DataandSwift.Stringprojections by adding methods required byCryptoKitand by using a struct forToStringcallback context.Additionally, the
CryptoKittests have been updated. TheFoundation.cshas been removed, andCryptoKit.cshas been refactored to align more closely with the tooling output.Out of scope
This PR doesn't fully replace
CryptoKit.cswith generated bindings. The follow-up work will be done as outlined in #2999.Fixes #2992