Skip to content

Documentation for Span<T> #194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
May 23, 2018
Merged

Documentation for Span<T> #194

merged 12 commits into from
May 23, 2018

Conversation

rpetrusha
Copy link

@rpetrusha rpetrusha commented May 17, 2018

Documentation for Span

Fixes dotnet/docs#4400

<remarks>
<format type="text/markdown"><![CDATA[

`Current` is undefined under any of the following conditions:
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the exception is documented, it's better to say:
Current throws an <xref:System.IndexOutOfRangeException> under any of the following conditions:

Interesting that's the IndexOutOfRangeException rather than InvalidOperationException.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it is interesting. And you're right, @pkulikov, about replacing the "undefined" text.


`Current` is undefined under any of the following conditions:

- Immediately after the enumertor is created, the enumerator is positioned before the first element in the span. <xref:System.Span%601.Enumerator.MoveNext%2A> must be called to advance the enumerator to the first element of the collection before reading the value of `Current`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it fine to refer to Span<T> as collection, as in the last sentence? Also given that in the first sentence it's referred as the span.

Copy link
Author

Choose a reason for hiding this comment

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

No, it's not, @pkulikov. Thanks for catching it.

@rpetrusha rpetrusha changed the title [WIP] Documentation for Span<T> Documentation for Span<T> May 21, 2018
Copy link
Member

@BillWagner BillWagner left a comment

Choose a reason for hiding this comment

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

Overall, this is a good start.

I had one comment, and you can decide if you think it's a good addition, or unnecessary.

Then, :shipit: when ready.

<remarks>
<format type="text/markdown"><![CDATA[

The C# [foreach](~/docs/csharp/language-reference/keywords/foreach-in.md) of the C# language and the [For Each...Next](~/docs/visual-basic/language-reference/statements/for-each-next-statement.md) construct in Visual Basic hides the complexity of enumerators. Instead of directly manipulating the enumerator, using `foreach` or `For Each...Next` is recommended.
Copy link
Member

Choose a reason for hiding this comment

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

It may be worth mentioning these two points:

Span<T>.Enumerator does not implement IEnumarator<T>.
Span<T>.Enumerator does not have a Reset() method.

Copy link
Author

Choose a reason for hiding this comment

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

@BillWagner, thanks. These are definitely worth mentioning.

@rpetrusha rpetrusha merged commit 2fb91b3 into dotnet:master May 23, 2018
@rpetrusha rpetrusha deleted the span branch May 23, 2018 19:35
@ahsonkhan
Copy link
Contributor

Looks good to me, in general. Is there a way for me to read the docs in human readable form (without the xml tags)? The links return 404.

Also, do you plan to complete the docs for ReadOnlySpan/Enumerator as well?

@rpetrusha
Copy link
Author

I think that the 404s were a result of the published content being deleted, @ahsonkhan. At least, I think that that's what happens when a PR is merged. Sometime tomorrow, the Span docs should be available live on docs.microsoft.com.

@JRAlexander is going to do ReadOnlySpan and ReadOnlySpan.Enumerator.

If you'd like to review the docs once they're live, that would be good. This was a quick-and-dirty attempt to get some documentation online. Sometime next week, I'm planning to enhance the documentation with additional detail and some examples.

@mairaw
Copy link
Contributor

mairaw commented May 23, 2018

@ahsonkhan given you're an FTE, you can also see the content in the review site: https://review.docs.microsoft.com/en-us/dotnet/api/system.span-1?branch=master&view=netcore-2.1
https://review.docs.microsoft.com/en-us/dotnet/api/system.span-1.enumerator?view=netcore-2.1&branch=master

@ahsonkhan
Copy link
Contributor

cc @KrzysztofCwalina - FYI

<param name="length">To be added.</param>
<summary>To be added.</summary>
<param name="array">The source array.</param>
<param name="start">The index of the first element to include in the new <see cref="T:System.Span~1" />.</param>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is using the tilde intentional? T:System.Span~1 In other places we use T:System.Span``1

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch @ahsonkhan. No, they're not by design.

<returns>To be added.</returns>
<remarks>To be added.</remarks>
<param name="obj">Not supported.</param>
<summary>Calls to this method are not supported.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this match the documentation of the GetHashCode method? Both throw NotSupportedException.

Copy link
Author

Choose a reason for hiding this comment

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

I got a compiler error instead, @ahsonkhan. Is that expected?

Copy link
Contributor

@ahsonkhan ahsonkhan May 23, 2018

Choose a reason for hiding this comment

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

private static readonly byte[] _array = new byte[5];
static void Main(string[] args)
{
    Span<byte> span = _array;
    Console.WriteLine(span.Equals(_array));
}

This compiles and builds successfully. At runtime, it throws an exception. I believe that is the expectation:

Unhandled Exception: System.NotSupportedException: Equals() on Span and ReadOnlySpan is not supported. Use operator== instead.
   at System.Span`1.Equals(Object obj)
   at Sample1_Iterating.Program.Main(String[] args) in D:\Other\trash\PipelineTest\Program.cs:line 427

What compiler error are you seeing instead?

Copy link
Author

@rpetrusha rpetrusha May 29, 2018

Choose a reason for hiding this comment

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

Sorry, @ahsonkhan, I missed your question. If obj is not a Span<T>, the exception is thrown. But if obj is a Span<T>, I get compiler error CS1503: cannot convert from 'System.Span' to 'object'

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, maybe that's worth mentioning as well. Since span is a ref struct, it cannot be boxed and hence cannot be converted to object. So, the compiler will return such an error if you try to pass in a span to any method that expects an object.


If <xref:System.Span%601.Enumerator.MoveNext%2A> passes the end of the <xref:System.Span%601>, the enumerator is positioned after the last item in the <xref:System.Span%601>, and <xref:System.Span%601.Enumerator.MoveNext%2A> returns `false`. When the enumerator is at this position, subsequent calls to <xref:System.Span%601.Enumerator.MoveNext%2A> also return `false`. If the call to <xref:System.Span%601.Enumerator.MoveNext%2A> returns `false`, <xref:System.Span%601.Enumerator.Current> is undefined. You cannot set <xref:System.Span%601.Enumerator.Current> to the first item in the <xref:System.Span%601> again; you must create a new enumerator instance instead.

The enumerator does not have exclusive access to the <xref:System.Span%601>; therefore, enumerating through a span is intrinsically not a thread-safe procedure. To guarantee thread safety during enumeration, you can lock the span during the entire enumeration. To allow the span to be accessed by multiple threads for reading and writing, you must implement your own synchronization.
Copy link
Contributor

@ahsonkhan ahsonkhan May 23, 2018

Choose a reason for hiding this comment

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

To guarantee thread safety during enumeration, you can lock the span during the entire enumeration.

@stephentoub, is this true? Couldn't the underlying array still be modified by another thread, or is the implication that the user needs some synchronization mechanism during enumeration and modification of the array? Also, how do we "lock" the span (it isn't a reference type).

The following snippet has a race. To resolve it, we have to use a locking mechanism around both critical sections.
Output:
62
23
0
0
0

static void Main(string[] args)
{
    new Random(42).NextBytes(_array);
    Span<byte> span = _array;

    Thread thread = new Thread(delegate ()
    {
        ClearContents();
    });

    thread.Start();

    EnumerateSpan(span);
}

private static readonly byte[] _array = new byte[5];

public static void ClearContents()
{
    Thread.Sleep(20);
    lock (_array)
    {
        Array.Clear(_array, 0, _array.Length);
    }
}

public static void EnumerateSpan(Span<byte> span)
{
    //lock (_array)
    //{
        foreach (byte element in span)
        {
            Console.WriteLine(element);
            Thread.Sleep(10);
        }
    //}
}

Copy link
Member

Choose a reason for hiding this comment

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

First and foremost, there's no way to "lock the span".

Presumably this was really just meant to say that you can use your own synchronization to enforce mutual exclusion.


Enumerators can be used to read the data in the <xref:System.Span%601>, but they cannot be used to modify it.

Initially, the enumerator is positioned before the first element in the <xref:System.Span%601>. At this position, <xref:System.Span%601.Enumerator.Current> is undefined, so you must call <xref:System.Span%601.Enumerator.MoveNext%2A> to advance the enumerator to the first item in the <xref:System.Span%601> before reading the value of <xref:System.Span%601.Enumerator.Current>.
Copy link
Contributor

Choose a reason for hiding this comment

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

At this position, Current throws an exception (it isn't undefined exactly).

@ahsonkhan
Copy link
Contributor

ahsonkhan commented May 23, 2018

cc'ing others if they have any input to improve the readability/correctness of the span docs that were added here: @dotnet/corefxlab-contrib, @joshfree

<returns>To be added.</returns>
<param name="arraySegment">The array segment to be converted to a <see cref="T:System.Span`1" />.</param>
<param name="segment">The array segment to be converted to a <see cref="T:System.Span`1" />.</param>
<summary>Defines an implicit conversion of an <see cref="T:System.ArraySegment" /> to a <see cref="T:System.Span`1" />.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

The xml tag around ArraySegment needs an adjustment. It isn't showing up as a link.

<summary>To be added.</summary>
<returns>To be added.</returns>
<remarks>To be added.</remarks>
<summary>Returns a reference to the element of the <see cref="T:System.Span`1" /> at index zero.</summary>
Copy link
Contributor

@ahsonkhan ahsonkhan May 23, 2018

Choose a reason for hiding this comment

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

The return type needs to be ref T, not T. Similary, span is a ref struct, not a regular struct.

@rpetrusha, do we generally make a distinction within the docs when a method is marked with an EditorBrowsableState.Never attribute?

@jkotas, should we call out the fact that this method is unsafe within the docs?

Copy link
Member

Choose a reason for hiding this comment

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

The doc should say that this method is there to support C# fixed statement.

There is nothing unsafe about this method (calling this method directly won't let you violate type or memory safety). I do not think the doc for this method needs to say anything about safety.

We have moved all unsafe Span-related methods to System.Runtime.InteropServices namespace. The documentation for those should mention that they are unsafe.

Copy link
Member

Choose a reason for hiding this comment

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

/cc @dend Do you pull the ref from the return type, or make a distinction between struct and ref struct when processing the ref assemblies?

<remarks>
<format type="text/markdown"><![CDATA[

If the source and `destination` overlap, this method places the original values in a temporary location before `destination` is overwritten.
Copy link
Contributor

@ahsonkhan ahsonkhan May 24, 2018

Choose a reason for hiding this comment

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

The API doesn't actually place the original values in a temporary location. It only behaves like this, semantically. The way it is currently written, it conveys the method logic imprecisely.

this method behaves as if the original values are in a temporary location before the destination is overwritten.


- Does not implement the <xref:System.Collections.IEnumerator> or <xref:System.Collections.Generic.IEnumerator%601> interface.

- Does not include a `Reset` method, which sets the enumerator to its initial position before the first element in the span.
Copy link
Member

Choose a reason for hiding this comment

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

Most enumerators provide Reset as it's part of the interface but throw or otherwise don't actually implement it.


Unlike some other enumerator structures in .NET, <xref:System.Span%601.Enumerator>:

- Does not implement the <xref:System.Collections.IEnumerator> or <xref:System.Collections.Generic.IEnumerator%601> interface.
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth mentioning that it's a ref struct and thus can't implement interfaces.

<summary>To be added.</summary>
<value>To be added.</value>
<remarks>To be added.</remarks>
<summary>Gets the item at the current position of the enumerator.</summary>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe Gets a reference to the item rather than Gets the item?

<summary>To be added.</summary>
<remarks>To be added.</remarks>
<typeparam name="T">The type of items in the <see cref="System.Span`1" />.</typeparam>
<summary>Represents a contiguous region of arbitrary memory that is type- and memory-safe.</summary>
Copy link
Member

Choose a reason for hiding this comment

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

This makes it sound like the memory being represented is type- and memory-safe, rather than it being the representation that is type- and memory-safe.

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

Successfully merging this pull request may close these issues.

[Feature] Span<T>
8 participants