Skip to content
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

Change ReadOnlySpan indexer to return ref readonly #23582

Closed
ektrah opened this issue Sep 17, 2017 · 9 comments
Closed

Change ReadOnlySpan indexer to return ref readonly #23582

ektrah opened this issue Sep 17, 2017 · 9 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Milestone

Comments

@ektrah
Copy link
Member

ektrah commented Sep 17, 2017

 namespace System
 {
     public struct ReadOnlySpan<T>
     {
         ...
-        public T this[int index] { get; }
+        public ref readonly T this[int index] { get; }
         ...
     }
 }

See https://github.com/dotnet/corefx/issues/23881#issuecomment-328741859.

@KrzysztofCwalina
Copy link
Member

@jkotas, @ahson, @jaredpar, @VSadov, thoughts?

@stephentoub
Copy link
Member

Sounds fine to me.

@VSadov
Copy link
Member

VSadov commented Sep 20, 2017

Good change.
Will avoid copying of the whole element in cases like.

ReadonlySpan<HugeNestedStruct> h =  ObtainSomehow();

int iOnlyNeedAnInt = h[42].Some.Deep.Nested.IntField;

@jkotas
Copy link
Member

jkotas commented Sep 20, 2017

Sounds fine to me.

@KrzysztofCwalina
Copy link
Member

Great. Thanks!

@jaredpar
Copy link
Member

Yes def should do this

@karelz
Copy link
Member

karelz commented Oct 10, 2017

FYI: The API review discussion was recorded - see https://youtu.be/b96co3sNzNI?t=2423 (2 min duration)

@ahsonkhan
Copy link
Contributor

Can someone please explain the distinction between ref readonly and readonly ref?

There is inconsistency in this particular issue, the title says:

Change ReadOnlySpan indexer to return readonly ref

The API change proposed (which is probably correct) says:
public ref readonly T this[int index] { get; }

@ektrah, can you please update the title?

Returning readonly ref throws compiler error:

The modifier 'readonly' is not valid for this item

The indexer should return ref readonly T while Span<T> is a readonly ref struct.

cc @VSadov

@ektrah ektrah changed the title Change ReadOnlySpan indexer to return readonly ref Change ReadOnlySpan indexer to return ref readonly Oct 27, 2017
@ektrah
Copy link
Member Author

ektrah commented Oct 27, 2017

Oops, sorry for the confusion. I've updated the title.

(It's really confusing that read-only references are denoted by ref readonly.)

jkotas referenced this issue in dotnet/corert Dec 14, 2017
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Projects
None yet
Development

No branches or pull requests

9 participants