-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add GetPosition overload to ReadOnlySequence #27633
Conversation
SequencePosition c1 = buffer.GetPosition(buffer.Start, 0); | ||
Assert.Equal(0, c1.GetInteger()); | ||
Assert.Equal(bufferSegment1, c1.GetObject()); | ||
{ |
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.
Split into two tests
SequencePosition c1 = buffer.GetPosition(buffer.Start, 0); | ||
Assert.Equal(0, c1.GetInteger()); | ||
Assert.Equal(bufferSegment1, c1.GetObject()); | ||
{ |
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.
Same here
@@ -24,11 +24,17 @@ public void SegmentStartIsConsideredInBoundsCheck() | |||
|
|||
var buffer = new ReadOnlySequence<byte>(bufferSegment1, 0, bufferSegment2, 50); | |||
|
|||
SequencePosition c1 = buffer.GetPosition(buffer.Start, 25); // segment 1 index 75 | |||
SequencePosition c2 = buffer.GetPosition(buffer.Start, 55); // segment 2 index 5 | |||
SequencePosition c1 = buffer.GetPosition(25); // segment 1 index 75 |
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 wonder if we should not call the API "PositionAt" (or GetPositionAt). It would allow us to add PositionOf(byte value) in the future without having problems with inconsistent names or overloads.
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.
We already have PositionOf(byte value)
extension method.
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.
That is my point. We will endup with GetPosition and PositionOf, which is inconsistent. It would be better to have either PositionAt and PositionOf or GetPositionAt and GetPositionOf.
@dotnet-bot test Windows x64 Debug Build |
@dotnet-bot test Linux x64 Release Build |
@dotnet-bot test Windows x64 Debug Build |
@@ -34,7 +34,7 @@ public void AdvanceThrowsIfFlushActiveAndNotConsumedPastThreshold() | |||
Assert.False(flushAsync.IsCompleted); | |||
|
|||
ReadResult result = _pipe.Reader.ReadAsync().GetAwaiter().GetResult(); | |||
SequencePosition consumed = result.Buffer.GetPosition(result.Buffer.Start, 31); | |||
SequencePosition consumed = result.Buffer.GetPosition(31, result.Buffer.Start); |
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.
Change this to just specify the length without the position for the ones that pass start.
@@ -19,7 +19,7 @@ public static class BuffersExtensions | |||
int index = sequence.First.Span.IndexOf(value); | |||
if (index != -1) | |||
{ | |||
return sequence.GetPosition(sequence.Start, index); | |||
return sequence.GetPosition(index, sequence.Start); |
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.
Remove sequence.Start
@dotnet-bot test OSX x64 Debug Build |
I am going to ignore the OSX and UWP legs (recently enabled - #27531) that are failing:
https://github.com/dotnet/core-eng/issues/2808 |
* Add GetPosition overload to ReadOnlySequence * Address feedback (split tests) * Use new GetPosition overload whenever passing start is redundant. Commit migrated from dotnet/corefx@9ff60da
Resolves https://github.com/dotnet/corefx/issues/27403
cc @pakrym, @davidfowl, @halter73, @KrzysztofCwalina