Skip to content
This repository has been archived by the owner on Aug 2, 2023. It is now read-only.

Generic implementation of Range. #1859

Closed
wants to merge 5 commits into from
Closed

Generic implementation of Range. #1859

wants to merge 5 commits into from

Conversation

YohDeadfall
Copy link
Contributor

At this moment the Range class isn't generic and stores int Index and int Length values, but probably it would be better to have a generic struct and move it to the System.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 inclusive T From and exclusive T To values), to manipulate collections and such related things.

Why the To field instead of Length? Because the To field can store floating point values without losing precision, and length can not be determined for some types like string. But length calculation can be made using extension methods for numeric types. Since we have the aggressively inlined and specialized Length(this Range<Int>) method, that change will not hurt performance.

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Oct 19, 2017

This complicates the type significantly. For example, the Slice method signature is currently Slice(Range range), but now it would be Slice(Range<int> range). Similarly on the caller side, it would complicate the callsite.

@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 Range or Range<T> is closer to what C# could (would like to) use?

@benaadams
Copy link
Member

benaadams commented Oct 19, 2017

Do you have any opinions on whether Range or Range is closer to

I think you need some back-ticks ` to show the <x> bits

@KrzysztofCwalina
Copy link
Member

@benaadams, I keep forgetting the ticks. Updated now.

@YohDeadfall
Copy link
Contributor Author

@KrzysztofCwalina, not everywhere. You forgot about the second Slice(Range range).

@Drawaes
Copy link
Contributor

Drawaes commented Oct 19, 2017

I can confirm that reading that in email was confusing... range range

@YohDeadfall
Copy link
Contributor Author

@KrzysztofCwalina, could you provide a link to the discussion with @jaredpar? Is it?

@KrzysztofCwalina
Copy link
Member

@YohDeadfall is was more of a hallway discussion.

@dotnet dotnet deleted a comment from dnfclas Oct 31, 2017
@dotnet dotnet deleted a comment from dnfclas Oct 31, 2017
Copy link
Member

@KrzysztofCwalina KrzysztofCwalina left a 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.

@YohDeadfall
Copy link
Contributor Author

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.

@mattjohnsonpint
Copy link
Contributor

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 T be constrained to IComparable<T> ?

@grant-d
Copy link

grant-d commented Nov 8, 2017

I posted (and deleted) some feedback based on my wrong assumption that the upper bound was inclusive.
I am curious why this design (of an exclusive upper bound) was chosen.
I understand for loops & their ilk are essentially [n, m) boundaries but I wonder whether users would expect discrete Ranges to behave the same.
In other words, my own expectation is:

//              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 enum specifying Inclusive/Exclusive (per Json ranges). This would mitigate the boundary issues with non-exact types such as double.

As an aside, Ranges are generally useful outside of System.Memory.
So it might be useful to include instance/extension methods like:

class Range<T>
{
  ...
  public bool InRange(T value) 
    => value >= From && value < To;
}

@YohDeadfall
Copy link
Contributor Author

@mj1856 Thanks. Don't know why I thought that the string class isn't IComparable<T>. Will fix it.

@grant-d They are already adapted for slices because exclusive ending ranges have length which equals to to - from (end - start) and slices get length as the second parameter. Also fully inclusive ranges cannot be empty, but exclusive ending ranges can therefore I chose them.

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)
    {
Range Length Slice Description
Range(0, 0) 0 Slice(0, 0) Take no values.
Range(1, 1) 0 Slice(1, 0) Take no values.
Range(1, 2) 1 Slice(1, 1) Take the first value starting 1, i.e. starting 1 until 2.
Range(1, 5) 1 Slice(1, 4) Take four values starting 1, i.e. starting 1 until 5.

An inclusive end makes sense because it allows to include the max value of T. So yes, ranges should be with exclusive ending and with inclusive ones as you suggested, but I don't think that everyone needs inclusive ranges. Adding a field to specify the type of the ending will increase the size of Range<T> and decrease performance. That's bad. Maybe developers should specify that in their API's by naming? For example:

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 inclusive checks.

@YohDeadfall
Copy link
Contributor Author

The C# slice syntax was discussed in #1306.

@YohDeadfall
Copy link
Contributor Author

YohDeadfall commented Nov 9, 2017

@mj1856 I remembered why T isn't constrained to IComparable<T>. It's because Nullable<T> has no interfaces. Should the constraint be added anyway? Do we need to support nullable?

@mattjohnsonpint
Copy link
Contributor

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 Range<DateTime>. Consider two meetings back-to-back. The first goes from 1:00 to 2:00. The second from 2:00 to 3:00. Which meeting is going on exactly at 2:00? Most people would generally say the second one, because the first meeting has ended. In other words, [1:00, 2:00) [2:00, 3:00) is the only rational way to think about ranges of time.

IMHO this should apply uniformly to all types.

@mattjohnsonpint
Copy link
Contributor

WRT Nullable<T>, I assume you mean for open-ended ranges like new Range<int?>(null, 10) or new Range<int?>(10, null) - but IMHO I don't think you need the T itself to be nullable to achieve that goal. Couldn't you just do that on the constructor? As in:

class Range<T>
    where T: struct, IComparable<T>
{
    public Range(T? from, T? to) { ... }
}

@YohDeadfall
Copy link
Contributor Author

Design overview

Type constraints

There are no constraints at all for two reasons:

  1. To support reference types and value types.
  2. To support Nullable<T> since it has no interfaces.
var range1 = new Range<long>(0, 100);
var range2 = new Range<long?>(null, 100);
var range3 = new Range<string>("a", "z");

Methods

I've implemented IsEmpty, IsNormalized, etc. as extension methods because:

  1. This way allows to write specialized methods even for non-BCL types and optimize performance.
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;
    // ...
}
  1. It's possible to provide a custom comparer to the methods. For example it's useful for strings.
var range = new Range<string>("A", "a");
var empty1 = range.IsEmpty(StringComparer.InvariantCulture); // false
var empty2 = range.IsEmpty(StringComparer.InvariantCultureIgnoreCase); // true

@mj1856 The Range<T> type could store the endings as T? but that will increase the size of Range<T>.

// 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;
}

@jaredpar
Copy link
Member

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.

@YohDeadfall
Copy link
Contributor Author

@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?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants