Corefxlab: Review Span<T> APIs

[KrzysztofCwalina]

We need to make APIs in coreclr and corefx consistent. Let's review the current APIs and decide on the shape we want.

CoreFxLab APIs:
c# public struct Span<T> : IEquatable<T[]> { public readonly int Length; public Span(object obj, UIntPtr offset, int length); // this would be best as Span(ref T object) public unsafe Span(void* ptr, int length); public Span(T[] array); public Span(T[] array, int start, int length); public static Span<T> Empty { get; } public bool IsEmpty { get; } public T this[int index] { get; set; } public void CopyTo(Span<T> destination); public void CopyTo(T[] destination); public override bool Equals(object obj); public bool Equals(ReadOnlySpan<T> other); public bool Equals(Span<T> other); public bool Equals(T[] other); public ReadOnlySpan<T>.Enumerator GetEnumerator(); public override int GetHashCode(); public static bool operator ==(Span<T> left, Span<T> right); public static implicit operator Span<T> (ArraySegment<T> arraySegment); public static implicit operator Span<T> (T[] array); public static bool operator !=(Span<T> left, Span<T> right); public bool ReferenceEquals(Span<T> other); public void Set(ReadOnlySpan<T> values); public void Set(T[] values); public Span<T> Slice(int start); public Span<T> Slice(int start, int length); public Span<T> Slice(uint start); public Span<T> Slice(uint start, uint length); public T[] ToArray(); }

We should discuss:

1. Remove interface impl?
2. Remove Slice(uint, uint) and Slice(uint)?
3. The enumerator is nested in ROSpan; looks strange here
4. Do we want Set(Span)?

[omariom]

1. Implies boxing
2. When you introduced them you mentioned it was more convenient in some (most?) cases. In parsing?
3. It could be just SpanEnumerator<T>
4. As both Spand and array implicitly convertible to ROS void Set(ReadOnlySpan<T> values); IMO enough

[omariom]

ref T this[int index] { get; } ?

[jkotas]

Length should be property for consistency.

[KrzysztofCwalina]

cc: @AtsushiKan

[ghost]

CoreClr currently has TryCopyTo() and not CopyTo() which seems strange to me as it only returns a failure for a condition the caller should be able to guarantee.

[ghost]

As mentioned in the discussion CR, GetHashCode() on Span is weird (and hard to implement in a good way in a portable library.)

[benaadams]

Remove Slice(uint, uint) and Slice(uint)?

Its really what all apis should be; what's a negative offset or negative length mean? However, sadly, it is incongruous with lengths and offsets are in .Net generally. Can always slice from memory etc twice without much added cost. Otherwise indexer should uint also - think should be removed?

[jkotas]

Need to have a way to get ref to the start to support pinning, e.g. ref T GetRawPointer().

[davidfowl]

• Remove public void CopyTo(T[] destination); since T[] is implicitly convertible to Span<T> there's no need for that overload.
• TryCopyTo vs CopyTo? CopyTo is what exists everywhere else in the BCL today
• I think Set is weird but maybe there's a use case I'm not thinking about? Just make a new Span?
• Do we need IsEmpty?

[nietras]

Support for asking if underlying memory is managed or native (e.g. pointer). And getting underlying object or pointer depending on which. Perhaps just via two methods e.g. TryGetObject, TryGetPointer or similar. Having the UIntPtr offset exposed as property would be nice as well. What is the reasoning for hiding these?

[jkotas]

What is the reasoning for hiding these?

They do not make sense on the fast version of Span<T>.

[benaadams]

Can range checks for T this[int index] be hoisted? e.g. using span[i] in a loop

[jkotas]

Can range checks for T this[int index] be hoisted? e.g. using span[i] in a loop

Yes, once the JIT optimizations part of https://github.com/dotnet/coreclr/issues/5851 is done.

[ghost]

ref T this[int index] { get; set; }

Hmm.. it seems C#7.0 is offering up a dissenting view...

Error CS8147 Properties which return by reference cannot have set accessors

[jkotas]

Right, the ref T indexer should be getter only.

[ghost]

Oh right, and then the deref happens on the callers side. Makes sense.

[KrzysztofCwalina]

This is done. The notes are at https://github.com/dotnet/apireviews/tree/master/2016/11-04-SpanOfT.
Thanks to all who participated!