Expose Index and Range types #20899
Expose Index and Range types #20899
Conversation
|
Have we looked at how this representation of Index and Range interacts with JIT optimizations? cc @AndyAyersMS |
| private readonly int _value; | ||
|
|
||
| public int Value => _value < 0 ? ~_value : _value; | ||
| public bool FromEnd => _value < 0; |
There was a problem hiding this comment.
Did we have an API review for this?
| } | ||
|
|
||
| public static implicit operator Index(int value) | ||
| => new Index(value, fromEnd: false); |
There was a problem hiding this comment.
Should these types have overridden Equals/GetHashCode/ToString like other core structs ?
There was a problem hiding this comment.
LDM hasn't talked about this that I can find, but I think it's a good idea.
There was a problem hiding this comment.
I'll add these. any suggestion for ToString return values?
I assume for Index we should return the index and ^ for from end, something like "1" and "^1"
For Range, we can return x..y and add ^ before the number if any of the indexes is from end.
what you think?
There was a problem hiding this comment.
I think that sounds good. cc @MadsTorgersen I'll bring this to LDM for confirmation, but you're good to put it in for preview 1.
There was a problem hiding this comment.
Should they implement IEquatable<T>?
There was a problem hiding this comment.
I'll go ahead and implement it.
|
please let me know if there is any more feedback. thanks. |
I have logged the issue https://github.com/dotnet/corefx/issues/33376 to track that. |
|
Thanks @stephentoub I have addressed all your feedback |
| } | ||
|
|
||
| return false; | ||
| } |
There was a problem hiding this comment.
Now that you've added the strongly typed Equals, this can just be:
public override bool Equals(object value) => value is Range && Equals((Range)value);but don't block your PR on it.
* Expose Index and Range types * Address Review Comments * Address more feedback * Addressing more comments Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Expose Index and Range types * Address Review Comments * Address more feedback * Addressing more comments Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Expose Index and Range types * Address Review Comments * Address more feedback * Addressing more comments Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Expose Index and Range types * Address Review Comments * Address more feedback * Addressing more comments Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
|
|
||
| namespace System | ||
| { | ||
| public readonly struct Index : IEquatable<Index> |
| @@ -154,6 +154,24 @@ internal ReadOnlySpan(ref T ptr, int length) | |||
| #endif | |||
| } | |||
|
|
|||
| public ref readonly T this[Index index] | |||
There was a problem hiding this comment.
Same here/elsewhere. Public APIs need documentation.
There was a problem hiding this comment.
I'll add the documentation later. thanks @ahsonkhan
|
|
||
| public override int GetHashCode() | ||
| { | ||
| return _value; |
There was a problem hiding this comment.
Should this GetHashCode() related to the FromEnd property?
There was a problem hiding this comment.
_value already include the from end info. look at how we initialize _value when having from end set.
* Expose Index and Range types * Address Review Comments * Address more feedback * Addressing more comments Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> (cherry picked from commit b730b3c)
* Expose Index and Range types * Address Review Comments * Address more feedback * Addressing more comments Commit migrated from dotnet/coreclr@3464b60
This change is exposing Index and Range types for C# 8.0 compiler support