-
Notifications
You must be signed in to change notification settings - Fork 344
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 implementation From 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
Range
class isn't generic and storesint Index
andint Length
values, but probably it would be better to have a generic struct and move it to theSystem.Collections.Sequences
library.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 From
and exclusiveT To
values), to manipulate collections and such related things.Why the
To
field instead ofLength
? Because theTo
field 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.