Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add Memory.Pin() to eventually replace Memory.Retain(bool)#28517

Closed
ahsonkhan wants to merge 4 commits into
dotnet:masterfrom
ahsonkhan:AddPin
Closed

Add Memory.Pin() to eventually replace Memory.Retain(bool)#28517
ahsonkhan wants to merge 4 commits into
dotnet:masterfrom
ahsonkhan:AddPin

Conversation

@ahsonkhan
Copy link
Copy Markdown

Depends on: dotnet/coreclr#17269

cc @KrzysztofCwalina, @jkotas, @davidfowl, @stephentoub, @dotnet/corefxlab-contrib

public static void MemoryFromEmptyArrayRetainWithPinning()
{
Memory<int> memory = new int[0];
MemoryHandle handle = memory.Retain(pin: true);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we adding tests for a method that's going away?

Copy link
Copy Markdown
Author

@ahsonkhan ahsonkhan Mar 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a reorder to be consistent and makes things easier (avoiding copy/paste errors).

<Compile Include="ReadOnlyMemory\Equality.cs" />
<Compile Include="ReadOnlyMemory\GetHashCode.cs" />
<Compile Include="ReadOnlyMemory\ImplicitConversion.cs" />
<Compile Include="ReadOnlyMemory\Pin.cs" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

corefx itself uses Retain in a bunch of places. Can those be switched over to Pin in this PR as well?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in progress.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread src/System.Memory/tests/Memory/Pin.cs Outdated
{
public static partial class MemoryTests
{

Copy link
Copy Markdown
Member

@davidfowl davidfowl Mar 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove extra space

@ahsonkhan
Copy link
Copy Markdown
Author

@dotnet-bot test this please

@ahsonkhan
Copy link
Copy Markdown
Author

Merged the commits from here into #28534.

@ahsonkhan ahsonkhan closed this Mar 29, 2018
@ahsonkhan ahsonkhan deleted the AddPin branch March 29, 2018 03:23
@karelz karelz added this to the 2.1.0 milestone Mar 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Memory blocked Issue/PR is blocked on something - see comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants