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

Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference#25936

Merged
ahsonkhan merged 7 commits into
dotnet:masterfrom
ahsonkhan:RemoveUseOfDanger
Dec 21, 2017
Merged

Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference#25936
ahsonkhan merged 7 commits into
dotnet:masterfrom
ahsonkhan:RemoveUseOfDanger

Conversation

@ahsonkhan
Copy link
Copy Markdown

@ahsonkhan ahsonkhan commented Dec 15, 2017

Part of:
https://github.com/dotnet/corefx/issues/25412
https://github.com/dotnet/corefx/issues/25615

Related to (and depends on): dotnet/coreclr#15532

Note: Expecting this failure for now until we have resolved whether we should add Unsafe.AsRef<T>(in T) or not. See: dotnet/coreclr#15532 (comment)

Cannot use method 'MemoryMarshal.GetReference<char>(ReadOnlySpan<char>)' as a ref or out value because it is a readonly variable

Following the staging plan from here: https://github.com/dotnet/corefx/issues/23881#issuecomment-343767740

  • Add MemoryExtensions.GetReference/TryGetArray
  • Convert all uses of DangerousGetPinnableReference/DangerousTryGetArray in coreclr, corefx, corert, corefxlab, aspnet, ... to MemoryExtensions.GetReference
  • Change DangerousGetPinnableReference to whatever we like to make it fit the pinning pattern and remove DangerousTryGetArray.

Doing it this way will avoid the need for complex staging or things being on the floor for extensive periods of time.

cc @jkotas, @stephentoub, @KrzysztofCwalina, @davidfowl, @pakrym, @joshfree

@ahsonkhan ahsonkhan self-assigned this Dec 15, 2017
@ahsonkhan ahsonkhan added the blocked Issue/PR is blocked on something - see comments label Dec 15, 2017
<Reference Include="System.Diagnostics.Contracts" />
<Reference Include="System.Diagnostics.Debug" />
<Reference Include="System.Diagnostics.Tools" />
<Reference Include="System.Memory" />
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 did you have to add these?

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.

MemoryMarshal is available through System.Memory.

@ahsonkhan ahsonkhan removed the blocked Issue/PR is blocked on something - see comments label Dec 16, 2017
private unsafe static string CombineNoChecksInternal(ReadOnlySpan<char> first, ReadOnlySpan<char> second, ReadOnlySpan<char> third)
{
Debug.Assert(first.Length > 0 && second.Length > 0 && third.Length > 0, "should have dealt with empty paths");
<<<<<<< HEAD
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.

@ahsonkhan Unresolved conflict

@ahsonkhan
Copy link
Copy Markdown
Author

Waiting on #25961 before this will pass CI.

@ahsonkhan ahsonkhan added the blocked Issue/PR is blocked on something - see comments label Dec 17, 2017
@jkotas jkotas removed the blocked Issue/PR is blocked on something - see comments label Dec 17, 2017
@jkotas
Copy link
Copy Markdown
Member

jkotas commented Dec 17, 2017

@dotnet-bot test this please

1 similar comment
@KrzysztofCwalina
Copy link
Copy Markdown
Member

@dotnet-bot test this please

@ahsonkhan ahsonkhan merged commit 40cbd56 into dotnet:master Dec 21, 2017
@ahsonkhan ahsonkhan deleted the RemoveUseOfDanger branch December 21, 2017 19:06
@karelz karelz added this to the 2.1.0 milestone Dec 28, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…otnet/corefx#25936)

* Convert uses of the Dangerous APIs to use MemoryMarshal.GetReference

* Fix unresolved merge conflict.

* Add using directive

* Add missing using directives.

* Add references to System.Memory


Commit migrated from dotnet/corefx@40cbd56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants