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

Added IsValid, Bind, and BindToValid. Renamed Last to End #1981

Merged
merged 2 commits into from
Dec 15, 2017
Merged

Added IsValid, Bind, and BindToValid. Renamed Last to End #1981

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
48f5e45
Added IsValid, Bind, and BindToValid
KrzysztofCwalina Dec 14, 2017
7a01e8e
PR feedback
KrzysztofCwalina Dec 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 91 additions & 18 deletions src/System.Buffers.Experimental/System/Range.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,23 @@

using System.Collections;
using System.Collections.Generic;
using System.Text;

namespace System
{
// TODO: consider allowing Last > First. Ennumeration will count down.
public struct Range : IEnumerable<int>
public readonly struct Range : IEnumerable<int>
{
public const int UnboundedFirst = Int32.MinValue;
public const int UnboundedLast = Int32.MaxValue;
public const int UnboundedEnd = Int32.MaxValue;

public readonly int First;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First should be renamed to Start to follow naming convention as @jcouv mentioned earlier in the previous PR: Start and End.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I am not thrilled with Start. It sounds like an action is about to start. That's why I originally did not use End. But I will think about the naming a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @YohDeadfall as Start seems to fit with End. However, I also see @KrzysztofCwalina point. I like First and Last better, though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Begin would be fine, but unfortunately it shouldn't be used for historical reasons since everywhere in .NET only Start/End is used for such situations in parameter names, fields, properties. Also Begin and End are used for pre-Task async operations in method names.

public readonly int Last; // Last is exclusive
public readonly int End; // End is exclusive

public uint Length
{
get {
if (IsBound) return (uint)((long)Last - (long)First);
if (IsBound) return (uint)((long)End - (long)First);
throw new InvalidOperationException("cannot get length of unbound range");
}
}
Expand All @@ -29,42 +30,42 @@ public Range(int first, uint length)
throw new ArgumentOutOfRangeException(nameof(first));

First = first;
Last = (int)(first + length);
End = (int)(first + length);

if (Last < First) throw new ArgumentOutOfRangeException(nameof(length));
if (End < First) throw new ArgumentOutOfRangeException(nameof(length));
}

private Range(int first, int last)
private Range(int first, int end)
{
First = first;
Last = last;
End = end;
}

public bool IsBound => First != UnboundedFirst && Last != UnboundedLast;
public bool IsBound => First != UnboundedFirst && End != UnboundedEnd;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't First == UnboundedFirst? I thought only End is exclusive.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The property returns true if both of the ends are bound. Otherwise false. The property can be used to see if some operations on the range would work, e.g. GetEnumerator works only for bound ranges. "Unbound" means "I don't know/care what the value is". It does not make sense to enumerate such ranges.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So, you can't enumerate from [int.MinValue..int.MinValue + 5] similar to int.MaxValue?

int.MaxValue being the sentinel for End makes sense given End is exclusive (for example [0..int.MaxValue] works fine. Are we treating the lower bound the same way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IsBound should take into account that the range could be reversed:

  • Range(UnboundedLast, UnboundedFirst) is unbound;
  • Range(5, UnboundedFirst) is unbound;
  • Range(UnboundedLast, 5) is unbound too.


public void Deconstruct(out int first, out int last)
public void Deconstruct(out int first, out int end)
{
first = First;
last = Last;
end = End;
}
public static Range Construct(int first, int last)
=> new Range(first, last);
public static Range Construct(int first, int end)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add new line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

=> new Range(first, end);

public struct Enumerator : IEnumerator<int>
{
int _current;
int _last;
int _end;

internal Enumerator(int first, int last)
internal Enumerator(int first, int end)
{
_current = first - 1;
_last = last;
_end = end;
}

public bool MoveNext()
{
_current++;
return _current < _last;
return _current < _end;
}

public int Current => _current;
Expand All @@ -79,7 +80,7 @@ void IDisposable.Dispose() { }
// TODO: write benchmark for this
public Enumerator GetEnumerator()
{
if(IsBound) return new Enumerator(First, Last);
if(IsBound) return new Enumerator(First, End);
throw new InvalidOperationException("cannot enumerate unbound range");
}

Expand All @@ -88,5 +89,77 @@ IEnumerator<int> IEnumerable<int>.GetEnumerator()

IEnumerator IEnumerable.GetEnumerator()
=> GetEnumerator();

public bool Contains(int value)
{
if (!IsBound) throw new InvalidOperationException("Unbound ranges cannot contain");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's unbound, then it contains any value. If it has unbound start, check the end only. Same for ranges with unbound start.

if (IsBound) return Start <= value && value < End;
if (Start == UnboundStart) return value < End;
if (End == UnboundEnd) return Start <= value;
return false;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not forget to check direction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically yes, but what are the scenarios for calling Contains on unbound ranges?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exceptions are okay for GetEnumerator since we can't iterate over infinity sequences, but Contains can work and therefore should. Otherwise, it wouldn't be useful, and developers would be managed to write they own implementation. For example:

class Example
{
    private Range _range;

    public Example(Range range) => _range = range;

    public void DoSomething(int value)
    {
        if (_range.Contains(value))
        {
        }
        else
        {
        }
    }
}

What if range provided to the constructor is unbound? Should I use (_range.Start == Range.UnboundStart || _range.Start <= value) && (_range.End == Range.UnboundEnd || value < _range.End) to know if the value is in _range to be sure that the code will not throw?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the in pattern matching will use Contains.

switch (variable)
{
    case in ..0: // should not throw
        break;
    // ...
    case in 5..: // should not throw
        break;
}

return value >= First && value < End;
}

/// <summary>
/// Returns true if this Range is a valid range for a zero based index of sspecified length.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spelling - sspecified

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix. thanks

/// </summary>
/// <param name="length">zero based length.</param>
/// <returns></returns>
public bool IsValid(int length)
{
if (First == UnboundedFirst)
{
if (End == UnboundedEnd) return true;
return End <= length;
}
else // First is bounded
{
if (First < 0) return false;
if (End == UnboundedEnd) return First <= length;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should First be < length here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it so that range can start after the last element. This is to allow empty ranges.

if (First > length) return false;
return End <= length;
}

throw new NotImplementedException();
Copy link
Member

@ahsonkhan ahsonkhan Dec 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we ever reach here? It doesn't look like it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nah, I will remove.

}

public Range Bind(int length)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add a comment here explaining what Bind is meant to do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

{
if (!IsValid(length)) throw new ArgumentOutOfRangeException(nameof(length));
if (IsBound) return this;

int first = 0;
if (First != UnboundedFirst) first = First;

int end;
if (End != UnboundedEnd) end = End;
else end = length;

return new Range(first, end);
}

/// <summary>
/// Binds the range possibly adjusting it to fit in the length.
/// </summary>
/// <param name="length">zero based length.</param>
/// <returns></returns>
public Range BindToValid(int length)
{
int first = First;
if (first < 0) first = 0;
if (first > length - 1) first = length;

int end = End;
if (end == UnboundedEnd || end > length) end = length;

return new Range(first, end);
}

public override string ToString()
{
var sb = new StringBuilder();
sb.Append('[');
if (First != UnboundedFirst) sb.Append(First);
sb.Append("..");
if (End != UnboundedEnd) sb.Append(End);
sb.Append(']');
return sb.ToString();
}
}
}
73 changes: 54 additions & 19 deletions tests/System.Buffers.Experimental.Tests/RangeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public void Basics()

Assert.Equal(first, range.First);
Assert.Equal(length, range.Length);
Assert.Equal(length + first, range.Last);
Assert.Equal(length + first, range.End);

long sum = 0;
uint numberOfItems = 0;
Expand All @@ -31,35 +31,35 @@ public void Basics()
}

Assert.Equal(numberOfItems, range.Length);
Assert.Equal((range.First + (long)range.Last - 1) * range.Length / 2, sum);
Assert.Equal((range.First + (long)range.End - 1) * range.Length / 2, sum);

(int f, int l) = range;
Assert.Equal(range.First, f);
Assert.Equal(range.Last, l);
Assert.Equal(range.End, l);

var constructed = Range.Construct(first, range.Last);
var constructed = Range.Construct(first, range.End);
Assert.Equal(range.First, constructed.First);
Assert.Equal(range.Last, constructed.Last);
Assert.Equal(range.End, constructed.End);
Assert.Equal(range.Length, constructed.Length);
}
}
}

[Fact]
public void FirstIsInclusiveLastIsExclusive()
public void FirstIsInclusiveEndIsExclusive()
{
var range = Range.Construct(10, 15);
Assert.Equal(10, range.First());
Assert.Equal(14, range.Last());
}

[Fact]
public void UnboundedLast()
public void UnboundedEnd()
{
var unboundedLast = Range.Construct(10, Range.UnboundedLast);
Assert.False(unboundedLast.IsBound);
Assert.Equal(10, unboundedLast.First);
Assert.Equal(Range.UnboundedLast, unboundedLast.Last);
var unboundedEnd = Range.Construct(10, Range.UnboundedEnd);
Assert.False(unboundedEnd.IsBound);
Assert.Equal(10, unboundedEnd.First);
Assert.Equal(Range.UnboundedEnd, unboundedEnd.End);
}

[Fact]
Expand All @@ -68,16 +68,51 @@ public void UnboundedFirst()
var unboundedFirst = Range.Construct(Range.UnboundedFirst, 10);
Assert.False(unboundedFirst.IsBound);
Assert.Equal(Range.UnboundedFirst, unboundedFirst.First);
Assert.Equal(10, unboundedFirst.Last);
Assert.Equal(10, unboundedFirst.End);
}

[Fact]
public void Unbounded()
{
var unbounded = Range.Construct(Range.UnboundedFirst, Range.UnboundedLast);
var unbounded = Range.Construct(Range.UnboundedFirst, Range.UnboundedEnd);
Assert.False(unbounded.IsBound);
Assert.Equal(Range.UnboundedFirst, unbounded.First);
Assert.Equal(Range.UnboundedLast, unbounded.Last);
Assert.Equal(Range.UnboundedEnd, unbounded.End);
}

[Theory]
[InlineData(0, 0, 0, true)]
[InlineData(0, 1, 1, true)]
[InlineData(0, 1, 0, false)] // non empty range not valid for empty array
[InlineData(-1, 0, 0, false)] // lower bound negative
public void IsValidArrayRange(int first, int end, int length, bool result)
{
var range = Range.Construct(first, end);
Assert.Equal(result, range.IsValid(length));
}

[Theory]
[InlineData(Range.UnboundedFirst, 0, 0, 0, 0)]
[InlineData(Range.UnboundedFirst, 1, 10, 0, 1)]
[InlineData(0, Range.UnboundedEnd, 1, 0, 1)]
public void Bind(int first, int end, int length, int resultFirst, int resultEnd)
{
var range = Range.Construct(first, end);
var result = range.Bind(length);
Assert.Equal(resultFirst, result.First);
Assert.Equal(resultEnd, result.End);
}

[Theory]
[InlineData(1, 0, Range.UnboundedEnd, 1)]
[InlineData(1, 1, Range.UnboundedEnd, 0)]
[InlineData(1, 3, Range.UnboundedEnd, 0)]
public void BindToValid(int arrayLength, int first, int end, uint boundRangeLength)
{
var range = Range.Construct(first, end);
var result = range.BindToValid(arrayLength);
Assert.Equal(boundRangeLength, result.Length);
Assert.Equal(boundRangeLength == 0 ? arrayLength : first, result.First);
}

[Theory]
Expand All @@ -87,12 +122,12 @@ public void Unbounded()
[InlineData(int.MinValue + 1, 1, int.MinValue + 2)]
// full range
[InlineData(int.MinValue + 1, uint.MaxValue - 2, int.MaxValue -1)]
public void BoundaryConditions(int first, uint length, int last)
public void BoundaryConditions(int first, uint length, int end)
{
var empty = new Range(first, length);
Assert.Equal(length, empty.Length);
Assert.Equal(first, empty.First);
Assert.Equal(last, empty.Last);
Assert.Equal(end, empty.End);
}

[Fact]
Expand All @@ -106,7 +141,7 @@ public void Errors()
});
Assert.Throws<ArgumentOutOfRangeException>(() =>
{
// MaxValue is used as a sentinel for Last, so Last cannot endup being it.
// MaxValue is used as a sentinel for End, so End cannot endup being it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spelling endup

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix

var tooLong = new Range(int.MaxValue, 1);
});
Assert.Throws<InvalidOperationException>(() =>
Expand All @@ -118,7 +153,7 @@ public void Errors()
Assert.Throws<InvalidOperationException>(() =>
{
// Cannot enumerate unbound
var unbound = Range.Construct(1, Range.UnboundedLast);
var unbound = Range.Construct(1, Range.UnboundedEnd);
unbound.GetEnumerator();
});
Assert.Throws<InvalidOperationException>(() =>
Expand All @@ -130,7 +165,7 @@ public void Errors()
Assert.Throws<InvalidOperationException>(() =>
{
// Cannot get length on unbound
var unbound = Range.Construct(1, Range.UnboundedLast);
var unbound = Range.Construct(1, Range.UnboundedEnd);
var length = unbound.Length;
});
}
Expand Down