-
Notifications
You must be signed in to change notification settings - Fork 122
Add support for IBuffer.AsMemory() and Memory.AsBuffer(). #1535
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
b41c126 to
c9ee687
Compare
src/cswinrt/strings/additions/Windows.Storage.Streams/WindowsRuntimeBufferExtensions.cs
Show resolved
Hide resolved
src/cswinrt/strings/additions/Windows.Storage.Streams/WindowsRuntimeBufferExtensions.cs
Outdated
Show resolved
Hide resolved
src/cswinrt/strings/additions/Windows.Storage.Streams/WindowsRuntimeBufferExtensions.cs
Show resolved
Hide resolved
src/cswinrt/strings/additions/Windows.Storage.Streams/WindowsRuntimeBufferExtensions.cs
Outdated
Show resolved
Hide resolved
e6e13e1 to
4d0d9dc
Compare
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 think we should expose these APIs as is, because they are effectively unsafe, but hide that. Having a custom memory manager wrapping an IBuffer is equivalent to having a memory manager with a finalizer (because IBuffer will have a finalizer), which is explicitly not recommended specifically because it's unsafe.
We should definitely expose a way to convert between IBuffer and Memory<T> types, but I'm wondering whether it wouldn't be better to do so via an API that makes the fact they're unsafe explicit, and with proper documentation, so developers can know what to do. For instance, something like this:
public static class WindowsRuntimeMarshal
{
public static Memory<byte> AsMemoryUnsafe(IBuffer buffer);
}Just for context, pasting it here again, this is the scenario I'm worried about:
IBuffer buffer = GetSomeBuffer();
Span<byte> span = buffer.AsMemory().Span;
// <-- GC runs here
DoStuff(span); // Boom 💥Also cc. @tannergooding, @GrabYourPitchforks, thoughts? 🙂
Doesn't this apply equally to string? string buffer = GetSomeString();
Span<char> span = buffer.AsMemory().Span;
// <-- GC runs here
DoStuff(span); // Boom 💥 |
No, because a |
|
Superseded by #1556 |
Contributes to #824
WindowsRuntimeBuffer is now internally represented by a
Memory<byte>instead of abyte[]. Also simplified a few codepaths to use the CopyTo functionality from Span.AsMemory()follows the approach taken byToArray()in that the capacity of theIBufferis ignored and the Memory represents the length of the buffer.