-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Review System.Buffers.SequenceReader<T> proposals #30807
Comments
cc'ing interested parties based on the issues listed: |
IMO, the SequenceReader should not get any API added involving a SequencePosition parameter. Right now, it doesn't have any other - except the Position property. I think there should be a clear separation between ReadOnlySequence and SequenceReader APIs, where ReadOnlySequence is dealing with SequencePosition objects and SequenceReader using integer offsets. |
namespace System.Buffers
{
public ref struct SequenceReader<T>
{
// Optimized API to position the reader at the end of the sequence (much faster than what users can write)
public void AdvanceToEnd();
// Pairs with existing Span<T> UnreadSpan;
public readonly ReadOnlySequence<T> UnreadSequence { get; }
// Overloads for TryRead that allow reading out a given count rather than to some delimiter (as with existing
// API span out will slice if it can or allocate and copy if it has to).
public bool TryRead(int length, out ReadOnlySpan<T> value);
public bool TryRead(long length, out ReadOnlySequence<T> value);
// Peeking out T, while skipping. This is more performant than users can write (avoids rewinding).
public readonly bool TryPeek(long offset, out T value);
// Equivalent "Peek" versions. They need a offset as peeking doesn't advance the reader and rewinding is super expensive.
public readonly bool TryPeek(long offset, int length, out ReadOnlySpan<T> value);
public readonly bool TryPeek(long offset, long length, out ReadOnlySequence<T> value);
// Pairs with existing TryCopyTo(Span<T> destination), which does not advance the reader (neither does this)
public readonly bool TryCopyTo(long offset, Span<T> destination);
}
} |
Shouldn't the parameter names be Also, the existing methods have the |
Being consistent with existing APIs make sense to me (rename
Maybe. I can see changing the I would be fine with both changes, but curious to hear what others have to say (@terrajobst, @JeremyKuhne, @bartonjs). The suggestion is: public ref struct SequenceReader<T>
{
- public bool TryRead(int length, out ReadOnlySpan<T> value);
+ public bool TryRead(out ReadOnlySpan<T> span, int length);
- public bool TryRead(long length, out ReadOnlySequence<T> value);
+ public bool TryRead(out ReadOnlySequence<T> sequence, long length);
- public readonly bool TryPeek(long offset, out T value);
+ public readonly bool TryPeek(out T value, long offset);
- public readonly bool TryPeek(long offset, int length, out ReadOnlySpan<T> value);
+ public readonly bool TryPeek(out ReadOnlySpan<T> span, long offset, int length);
- public readonly bool TryPeek(long offset, long length, out ReadOnlySequence<T> value);
+ public readonly bool TryPeek(out ReadOnlySequence<T> sequence, long offset, long length);
} Here are the relevant existing APIs that shipped in 3.0 (for reference): public readonly bool TryPeek(out T value) { throw null; }
public bool TryRead(out T value) { throw null; }
public bool TryReadTo(out System.Buffers.ReadOnlySequence<T> sequence, System.ReadOnlySpan<T> delimiter, bool advancePastDelimiter = true) { throw null; }
public bool TryReadTo(out System.Buffers.ReadOnlySequence<T> sequence, T delimiter, bool advancePastDelimiter = true) { throw null; }
public bool TryReadTo(out System.Buffers.ReadOnlySequence<T> sequence, T delimiter, T delimiterEscape, bool advancePastDelimiter = true) { throw null; }
public bool TryReadTo(out System.ReadOnlySpan<T> span, T delimiter, bool advancePastDelimiter = true) { throw null; }
public bool TryReadTo(out System.ReadOnlySpan<T> span, T delimiter, T delimiterEscape, bool advancePastDelimiter = true) { throw null; }
public bool TryReadToAny(out System.Buffers.ReadOnlySequence<T> sequence, System.ReadOnlySpan<T> delimiters, bool advancePastDelimiter = true) { throw null; }
public bool TryReadToAny(out System.ReadOnlySpan<T> span, System.ReadOnlySpan<T> delimiters, bool advancePastDelimiter = true) { throw null; } |
|
There are some existing |
@ahsonkhan this meta issue has got approved label but seem that a lot of related issue are ready to review...can you confirm?I'd like to work on some of those but I don't know if make sense. |
@MarcoRossignoli- we need to get the other issues resolved again, so we can't do those yet. We need to revisit: dotnet/corefx#40780 and #30779. |
FYI, that should be #30778 (not dotnet/corefx#40780). |
Ok thanks! |
Closing the meta issue and tracking via the linked issues. |
This is a meta issue for #30779, #30778, #30771, #30770, and #29360 which add new APIs to
SequenceReader<T>
. This captures the overall change for the review. Detailed discussions are in the linked issues.Proposed new surface area:
The text was updated successfully, but these errors were encountered: