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. |
jkotas
left a comment
jkotas
left a comment
There was a problem hiding this comment.
Please make sure that we have follow up items to look at the performance of this
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 <[email protected]>
* Expose Index and Range types * Address Review Comments * Address more feedback * Addressing more comments Signed-off-by: dotnet-bot <[email protected]>
* Expose Index and Range types * Address Review Comments * Address more feedback * Addressing more comments Signed-off-by: dotnet-bot <[email protected]>
* Expose Index and Range types * Address Review Comments * Address more feedback * Addressing more comments Signed-off-by: dotnet-bot <[email protected]>
|
|
||
| namespace System | ||
| { | ||
| public readonly struct Index : IEquatable<Index> |
| #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 <[email protected]> (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
9 participants
This change is exposing Index and Range types for C# 8.0 compiler support