Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: long ReadOnlySequence<T>.GetPosition(SequencePosition) #27158

Closed
AArnott opened this issue Aug 16, 2018 · 8 comments · Fixed by #771
Closed

Feature request: long ReadOnlySequence<T>.GetPosition(SequencePosition) #27158

AArnott opened this issue Aug 16, 2018 · 8 comments · Fixed by #771
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Milestone

Comments

@AArnott
Copy link
Contributor

AArnott commented Aug 16, 2018

In implementing a Stream over a ReadOnlySequence<byte> (see #27156), I find to implement the Stream.Position property getter, I need to translate my SequencePosition into a long. It's easy enough to translate a long into a SequencePosition, but there's no built-in way to change it back.

I propose the addition of this API:

public struct ReadOnlySequence<T> {
   public long GetPosition(SequencePosition);
}
@benaadams
Copy link
Member

/cc @pakrym @davidfowl

@pakrym
Copy link
Contributor

pakrym commented Aug 16, 2018

How useful is Position implementation? I imagined that ROS-Stream wrapped would slice ROS for every ReadAsync and discard returned segment.

@AArnott
Copy link
Contributor Author

AArnott commented Aug 16, 2018

I imagined that ROS-Stream wrapped would slice ROS for every ReadAsync and discard returned segment.

A non-seekable would do that, yes. But if you wanted the stream to be seekable, which is certainly allowed given a ReadOnlySequence<byte>, then you'd need to know what position you were at and report that as a long given the Stream.Position contract.

@pakrym
Copy link
Contributor

pakrym commented Aug 16, 2018

As a workaround, you can do .Slice(0, pos).Length to get a position. You can also store a running index along with the current position.

@terrajobst
Copy link
Contributor

terrajobst commented Oct 1, 2019

Video

Given that the nomenclature uses offset for long-based absolute values and position for a SequencePosition value, we should be using GetOffset(). This also solves some overload issue that only @tannergooding understands 😄

namespace System
{
    public struct ReadOnlySequence<T>
    {
       public long GetOffset(SequencePosition position);
    }
}

@MarcoRossignoli
Copy link
Member

Are we sure about long or should be ulong the return value?In case of multisegment position is (long)RunningIndex+(int)SequencePosition.GetIndex()

@benaadams
Copy link
Member

long is 9223 petabytes moving to ulong doubles that to 18446 petabytes; they are both fairly big...

@MarcoRossignoli
Copy link
Member

MarcoRossignoli commented Dec 11, 2019

Yep, it's sure enough...formality when I see "on the paper possible" overflow.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 5.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants