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

KrzysztofCwalina · 2017-12-15T21:50:25Z

I added some helpers and overrode ToString to make it easier to work with ranges.
Renamed Last to End as "Last" is confusing given the value is exclusive.

@ahsonkhan, @joshfree, @eerhardt, @jcouv

ahsonkhan · 2017-12-15T21:53:14Z

src/System.Buffers.Experimental/System/Range.cs

        }

-        public bool IsBound => First != UnboundedFirst && Last != UnboundedLast;
+        public bool IsBound => First != UnboundedFirst && End != UnboundedEnd;


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

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.

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?

ahsonkhan · 2017-12-15T22:14:41Z

src/System.Buffers.Experimental/System/Range.cs

        }
-        public static Range Construct(int first, int last)
-            => new Range(first, last);
+        public static Range Construct(int first, int end)


nit: add new line

ahsonkhan · 2017-12-15T22:16:34Z

src/System.Buffers.Experimental/System/Range.cs

+            else // First is bounded
+            {
+                if (First < 0) return false;
+                if (End == UnboundedEnd) return First <= length;


Should First be < length here?

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

ahsonkhan · 2017-12-15T22:16:41Z

src/System.Buffers.Experimental/System/Range.cs

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


nit: spelling - sspecified

will fix. thanks

ahsonkhan · 2017-12-15T22:17:14Z

src/System.Buffers.Experimental/System/Range.cs

+                return End <= length;
+            }
+
+            throw new NotImplementedException();


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

nah, I will remove.

ahsonkhan · 2017-12-15T22:18:14Z

src/System.Buffers.Experimental/System/Range.cs

+            throw new NotImplementedException();
+        }
+
+        public Range Bind(int length)


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

ahsonkhan · 2017-12-15T22:22:04Z

tests/System.Buffers.Experimental.Tests/RangeTests.cs

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


nit: spelling endup

ahsonkhan

otherwise, LGTM.

YohDeadfall · 2017-12-15T22:15:25Z

src/System.Buffers.Experimental/System/Range.cs

    {
        public const int UnboundedFirst = Int32.MinValue;
-        public const int UnboundedLast = Int32.MaxValue;
+        public const int UnboundedEnd = Int32.MaxValue;

        public readonly int First;


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

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.

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

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.

YohDeadfall · 2017-12-15T22:28:09Z

src/System.Buffers.Experimental/System/Range.cs

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


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;

Do not forget to check direction.

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

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?

Also the in pattern matching will use Contains.

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

YohDeadfall · 2017-12-18T10:22:52Z

src/System.Buffers.Experimental/System/Range.cs

        }

-        public bool IsBound => First != UnboundedFirst && Last != UnboundedLast;
+        public bool IsBound => First != UnboundedFirst && End != UnboundedEnd;


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.

Added IsValid, Bind, and BindToValid

48f5e45

ahsonkhan reviewed Dec 15, 2017

View reviewed changes

ahsonkhan approved these changes Dec 15, 2017

View reviewed changes

YohDeadfall reviewed Dec 15, 2017

View reviewed changes

PR feedback

7a01e8e

KrzysztofCwalina force-pushed the RangeHelpers branch from 7c45655 to 7a01e8e Compare December 15, 2017 23:39

KrzysztofCwalina merged commit d68a9a4 into dotnet:master Dec 15, 2017

jcouv mentioned this pull request Dec 16, 2017

Test plan for slicing/Range feature dotnet/roslyn#23789

Closed

2 tasks

YohDeadfall reviewed Dec 18, 2017

View reviewed changes

KrzysztofCwalina deleted the RangeHelpers branch January 3, 2018 00:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

KrzysztofCwalina commented Dec 15, 2017

ahsonkhan Dec 15, 2017

KrzysztofCwalina Dec 15, 2017

ahsonkhan Dec 15, 2017

ahsonkhan Dec 15, 2017

KrzysztofCwalina Dec 15, 2017

ahsonkhan Dec 15, 2017

KrzysztofCwalina Dec 15, 2017

ahsonkhan Dec 15, 2017

KrzysztofCwalina Dec 15, 2017

ahsonkhan Dec 15, 2017 •

edited

Loading

KrzysztofCwalina Dec 15, 2017

ahsonkhan Dec 15, 2017

KrzysztofCwalina Dec 15, 2017

ahsonkhan Dec 15, 2017

KrzysztofCwalina Dec 15, 2017

ahsonkhan left a comment

YohDeadfall Dec 15, 2017

KrzysztofCwalina Dec 15, 2017

shaggygi Dec 16, 2017

YohDeadfall Dec 18, 2017

YohDeadfall Dec 15, 2017

YohDeadfall Dec 15, 2017

KrzysztofCwalina Dec 15, 2017

YohDeadfall Dec 18, 2017

YohDeadfall Dec 18, 2017

YohDeadfall Dec 18, 2017

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

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

Conversation

KrzysztofCwalina commented Dec 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Dec 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Dec 15, 2017 •

edited

Loading