Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

Add Span<T> and related APIs #819

Closed
wants to merge 7 commits into from
Closed

Conversation

terrajobst
Copy link

@dotnet/nsboard

terrajobst and others added 7 commits July 13, 2018 18:50
This allows compilers to mark structs as read-only.
This allows compilers to mark structs as by-ref-like (ref in C#).
The two existing types RuntimeArgumentHandle and TypedReference have always
been considered like the new ref-types -- it's just that the notion as been
generalized.
@terrajobst terrajobst added netstandard-api This tracks requests for standardizing APIs. runtime-impact Marks API suggestions that require runtime changes labels Jul 14, 2018
@terrajobst terrajobst added this to the .NET Standard vNext milestone Jul 14, 2018

namespace System.Buffers
{
public abstract partial class ArrayPool<T>
Copy link
Member

Choose a reason for hiding this comment

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

Is ArrayPool related to Span?

@@ -260,7 +260,7 @@ public partial class KeyNotFoundException : System.SystemException, System.Runti
public KeyNotFoundException(string message, System.Exception innerException) { }
}
[System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]
public partial struct KeyValuePair<TKey, TValue>
public readonly partial struct KeyValuePair<TKey, TValue>
Copy link
Member

Choose a reason for hiding this comment

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

This isn't directly related to Span.

@weshaggard
Copy link
Member

weshaggard commented Jul 26, 2018

Perhaps I'm letting the PR title distract me but I see a number of changes unrelated to Span.

@weshaggard
Copy link
Member

@ahsonkhan @KrzysztofCwalina @stephentoub @joshfree could you please take a look at the potential .NET Standard API additions related to Span.

@terrajobst
Copy link
Author

terrajobst commented Jul 26, 2018

Perhaps I'm letting the PR title distract me but I see a number of changes unrelated to Span.

See commit history. I had to add support for a couple of things. Maybe I should split the PR. The downside is that this needs more branches, but I think that's fine.

@terrajobst terrajobst requested review from a team October 12, 2018 22:37
InvalidData = 3,
NeedMoreData = 2,
}
[System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]

Choose a reason for hiding this comment

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

Isn't this implementation details?

public System.Buffers.ReadOnlySequence<T> Slice(System.SequencePosition start, System.SequencePosition end) { throw null; }
public override string ToString() { throw null; }
public bool TryGet(ref System.SequencePosition position, out System.ReadOnlyMemory<T> memory, bool advance = true) { memory = default(System.ReadOnlyMemory<T>); throw null; }
[System.Runtime.InteropServices.StructLayoutAttribute(System.Runtime.InteropServices.LayoutKind.Sequential)]

Choose a reason for hiding this comment

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

Same as above

@@ -434,4 +434,47 @@ public partial class UnobservedTaskExceptionEventArgs : System.EventArgs
public bool Observed { get { throw null; } }
public void SetObserved() { }
}
[System.Runtime.CompilerServices.AsyncMethodBuilderAttribute(typeof(System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder))]

Choose a reason for hiding this comment

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

This looks like redundant

Copy link

Choose a reason for hiding this comment

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

@marek-safar As I understand it, AsyncMethodBuilderAttribute is used by the C# compiler to figure out how to generate an async method that returns the given type (here, ValueTask). So I think it should be included in reference assemblies.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's required. The compiler uses it to determine what builder type to use for async methods that return the annotated type.

@terrajobst
Copy link
Author

I'm going to split this PR into more reviewable chunks.

@terrajobst terrajobst closed this Oct 16, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
netstandard-api This tracks requests for standardizing APIs. runtime-impact Marks API suggestions that require runtime changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants