search

dotnet / dotnet-api-docs

.NET API reference documentation (.NET 5+, .NET Core, .NET Framework)
https://docs.microsoft.com/dotnet/api/
Other
725 stars 1.56k forks source link

Span<T> indexer getter and setter, or only getter? #1533

Open elgonzo opened 5 years ago

elgonzo commented 5 years ago

The documentation can't make up its mind whether the Span<T> indexer only has a getter or both a getter and a setter.

It starts with "Gets or sets the element at the specified zero-based index.", directly followed by showing the declaration having only a getter.

If the availability of the setter depends on certain platform versions, then this should be made clear in this documentation page.

Document Details

⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

svick commented 5 years ago

The indexer has a ref return, which means that it only has a getter, but that getter can be used to both get and set the value (and also to store a reference to it in a variable).

This is confusing because:

  1. The signature is incorrect. It's shown as public T this[int index] { get; } when it should be public ref T this[int index] { get; }. I believe this issue should be fixed soon, once the fix for https://github.com/mono/api-doc-tools/issues/249 propagates here.

  2. The description is the same as for a regular property with both getter and setter. Since ref returns are different, maybe the description should be different too? What about the following?

    Returns a reference to the element at the specified zero-based index.

mairaw commented 5 years ago

Thanks for pointing out the incorrect signature @svick. I'll run the new CI release then once the new version is out.

I think we should continue to use the word Gets as we usually do for properties but maybe say "Gets a reference to the element..."

Thoughts?

mairaw commented 5 years ago

@joelmartinez the release notes for mdoc indicate that issue https://github.com/mono/api-doc-tools/issues/249 should have been fixed by version 5.7.4.7 but we're still seeing the old signature. Do we need to reopen 249? /cc @TianqiZhang

@svick what do you think of my proposal?

joelmartinez commented 5 years ago

@mairaw hmm, well, it seems that the test case for this particular mdoc issue was a bit too focused … it didn't include an indexer in this configuration, only:

    public ref int Ref() => ref i;
    public ref readonly int ReadonlyRef() => ref i;

Since this (strictly speaking) implemented the requirements given in https://github.com/mono/api-doc-tools/issues/249, please open a new GH issue to add ref and ref readonly support for indexers

mairaw commented 5 years ago

@rpetrusha @BillWagner I'd like to come up with a convention for this kind of API so we can add that to the style guide when we create that.