Generic implementation of Range.#1859
Generic implementation of Range.#1859YohDeadfall wants to merge 5 commits intodotnet:masterfrom YohDeadfall:master
Conversation
|
This complicates the type significantly. For example, the Slice method signature is currently @jaredpar, I originally added the Range type to play with the idea we talked about where C# slice syntax would take a range type. Do you have any opinions on whether |
I think you need some back-ticks ` to show the |
|
@benaadams, I keep forgetting the ticks. Updated now. |
|
@KrzysztofCwalina, not everywhere. You forgot about the second |
|
I can confirm that reading that in email was confusing... range range |
|
@KrzysztofCwalina, could you provide a link to the discussion with @jaredpar? Is it? |
|
@YohDeadfall is was more of a hallway discussion. |
KrzysztofCwalina
left a comment
There was a problem hiding this comment.
I think the generic parameter adds complexity for the most common scenario by far (range described by two ints) to enable a corner case scenario. And so, I don't think it's a good trade off.
|
I can close this pull request and file an issue in the corefx repository to invite more people to discuss, but before that it would be good to know what is the slice syntax. |
|
One thing - in order to be used as a range type, at some point you'll need to compare the extents of the range with other values - so shouldn't |
|
I posted (and deleted) some feedback based on my wrong assumption that the upper bound was inclusive. // Expected Actual (current design)
Range(0, 0); // ~equivalent to Empty ~equivalent to Empty
Range(1, 1); // ~equivalent to Slice(1, 1) ~equivalent to Empty (also)
Range(1, 2); // ~equivalent to Slice(1, 2) ~equivalent to Slice(1, 1)I believe an inclusive upper boundary would make more sense. And/or include an As an aside, class Range<T>
{
...
public bool InRange(T value)
=> value >= From && value < To;
} |
|
@mj1856 Thanks. Don't know why I thought that the @grant-d They are already adapted for slices because exclusive ending ranges have length which equals to public readonly ref struct Span<T>
{
public Span<T> Slice(int start, int length) { throw null; }
}
public readonly struct ReadOnlyBytes
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public ReadWriteBytes Slice(Range<int> range)
{
return Slice(range.From, range.Length());
}
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public ReadWriteBytes Slice(int index, int length)
{
An inclusive end makes sense because it allows to include the max value of void DoSomething(Range<int> inclusiveRange, Range<int> exclusiveRange) { }From our side we can provide additional extension methods: public static bool Contains<T>(this Range<T> range, T value); // exclusive by default *
public static bool ContainsInclusive<T>(this Range<T> range, T value); // inclusive by request
// or
public static bool Contains<T>(this Range<T> range, T value, bool inclusive = false);
public static double Length(this Range<double> range); // exclusive by default *
public static double LengthInclusive(this Range<double> range); // inclusive by request
// or
public static bool Contains<T>(this Range<T> range, bool inclusive = false);
// * in current implementationFrom that point we have the two types of ranges with zero cost. With aggressive inlining the compiler will eliminate |
|
The C# slice syntax was discussed in #1306. |
|
@mj1856 I remembered why |
|
Just to chime in on the subject of the exclusive upper bound, that's really important if they are to be used generically, such as IMHO this should apply uniformly to all types. |
|
WRT class Range<T>
where T: struct, IComparable<T>
{
public Range(T? from, T? to) { ... }
} |
Design overviewType constraintsThere are no constraints at all for two reasons:
var range1 = new Range<long>(0, 100);
var range2 = new Range<long?>(null, 100);
var range3 = new Range<string>("a", "z");MethodsI've implemented
public readonly struct TinyTime
{
private readonly byte _ticks;
// properties, operators and so on
}
public static class RangeExtensions
{
// everything with MethodImplOptions.AggressiveInlining
public static bool IsEmpty(this Range<TinyTime> range) => range.From == range.To;
public static bool IsNormalized(this Range<TinyTime> range) => range.From <= range.To;
// ...
}
var range = new Range<string>("A", "a");
var empty1 = range.IsEmpty(StringComparer.InvariantCulture); // false
var empty2 = range.IsEmpty(StringComparer.InvariantCultureIgnoreCase); // true@mj1856 The // Marshal.SizeOf(new Range<int>()) == 16
struct Range<T> where T: struct, IComparable<T>
{
public T? From;
public T? To;
}// Marshal.SizeOf(new Range<int>()) == 8
struct Range<T> where T: IComparable<T>
{
public T From;
public T To;
} |
|
The language design at this time does not intend to support a generic Range type. Rather we are focusing on concrete types and operator extensibility to support additional ranges. Do not think we should take this change as it's going the opposite direction as the language / compiler. |
|
@jaredpar Okay, then I'll close this pull request. But could you post your response to dotnet/roslyn#23205 because not only I think about generics? |
At this moment the
Rangeclass isn't generic and storesint Indexandint Lengthvalues, but probably it would be better to have a generic struct and move it to theSystem.Collections.Sequenceslibrary.The main idea is that
Range<T>could be used not only by buffers, but also in third party code to store a range of values (as inclusiveT Fromand exclusiveT Tovalues), to manipulate collections and such related things.Why the
Tofield instead ofLength? Because theTofield can store floating point values without losing precision, and length can not be determined for some types likestring. But length calculation can be made using extension methods for numeric types. Since we have the aggressively inlined and specializedLength(this Range<Int>)method, that change will not hurt performance.