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

[WIP] Don't Merge MemoryMarshal #14833

Closed
wants to merge 2 commits into from
Closed

Conversation

Drawaes
Copy link

@Drawaes Drawaes commented Nov 2, 2017

Just a start to check approach is correct.

@Drawaes
Copy link
Author

Drawaes commented Nov 2, 2017

For the span I can use the Unsafe methods to get the correct ref and avoid any bounds checks.

The memory is more complex, we end up with exactly the same overloads for the internal unsafe constructor and the external one. Flipping the parameters is a way to get around them unless someone else has a better idea?

/// <param name="start">The index at which to begin the span.</param>
/// <param name="length">The number of items in the span.</param>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static Memory<T> DangerousCreateMemory<T>(T[] array, int start, int length) => new Memory<T>(start, length, array);
Copy link
Member

Choose a reason for hiding this comment

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

The approved APIs do not have Dangerous in names...

Copy link
Author

Choose a reason for hiding this comment

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

I forgot that got removed.

Copy link
Member

@jkotas jkotas Nov 2, 2017

Choose a reason for hiding this comment

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

Actually, I do not see APIs like these in the approved list. Where did these exact APIs got approved?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Basically there are two API Reviewed Issues that have been approved that converge on this single type.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I have not noticed this API review. I do not think that the approved APIs are good.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I will hold off until that discussion has been had over there.

@Drawaes Drawaes closed this Nov 12, 2017
@Drawaes
Copy link
Author

Drawaes commented Nov 12, 2017

Closed, as other changes are being made here and currently tied up with SslStream

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants