Add SequenceReader<T> #33288
Add SequenceReader<T> #33288
Conversation
Initial add of ReadOnlySequence<T> reader. Includes search and binary read functionality. Perf tests will be ported from CoreFXlab later. Parse functionality still being discussed.
src/System.Memory/src/System/Buffers/ReadOnlySequence_helpers.cs
Outdated
Show resolved
Hide resolved
src/System.Memory/src/System/Buffers/SequenceReaderExtensions_binary.cs
Outdated
Show resolved
Hide resolved
src/System.Memory/src/System/Buffers/SequenceReaderExtensions_binary.cs
Outdated
Show resolved
Hide resolved
|
Update the package description? Line 1046 in 684704d |
| @@ -97,14 +97,10 @@ public static partial class MemoryExtensions | |||
| private readonly object _dummy; | |||
| private readonly int _dummyPrimitive; | |||
| public SequencePosition(object @object, int integer) { throw null; } | |||
| [System.ComponentModel.EditorBrowsableAttribute((System.ComponentModel.EditorBrowsableState)(1))] | |||
There was a problem hiding this comment.
Why did this end up getting removed? I would revert this change, build the ref and src, and then run the generate command from within the ref directory. Please let me know if you are still seeing such side effects/unintended changes.
There was a problem hiding this comment.
The build target did it.
There was a problem hiding this comment.
Manually added them back for now.
| } | ||
| public ref partial struct SequenceReader<T> where T : struct, System.IEquatable<T> | ||
| { | ||
| public SequenceReader(System.Buffers.ReadOnlySequence<T> buffer) { throw null; } |
There was a problem hiding this comment.
Regenerate based on new sources so that arg names match.
There was a problem hiding this comment.
Whoops, sorry, missed this comment. I'll fix shortly.
src/System.Memory/src/System/Buffers/SequenceReaderExtensions_binary.cs
Outdated
Show resolved
Hide resolved
|
It is never expected to throw.
From: Ahson Khan <notifications@github.com>
@ahsonkhan commented on this pull request.
…________________________________
In src/System.Memory/src/System/Buffers/SequenceReader.cs<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fdotnet%2Fcorefx%2Fpull%2F33288%23discussion_r231737626&data=02%7C01%7Cjeremy.kuhne%40microsoft.com%7C7a8e4f973e234c529a0208d64517f958%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636772366605577629&sdata=ExiLShzLyQ8NXJVL5s1daTlld8oQXFYnrWjR%2Fbw3FcA%3D&reserved=0>:
Yes, but is it expected for this to ever throw or is that an indication that something went wrong? I am not sure if that's the behaviour we want. And if it is, we should document exactly why we throw and why it's acceptable
|
Then, why not add the debug.assert and/or comment? |
src/System.Memory/src/System/Buffers/SequenceReaderExtensions_binary.cs
Outdated
Show resolved
Hide resolved
Because an assert here would give you no useful information. In any case, that code no longer exists as I removed |
… as we don't have the same pattern yet for the other overloads.
| { | ||
| int escapeCount = 0; | ||
| for (int i = remaining.Length; i > 0 && remaining[i - 1].Equals(delimiterEscape); i--, escapeCount++) | ||
| ; |
There was a problem hiding this comment.
Nit: there seems to be some inconsistency as to whether loops with empty bodies just use ; or have empty braces.
There was a problem hiding this comment.
Is this something we've called out in our guidelines? I'll massage it either way.
| /// Move the reader back the specified number of items. | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public void Rewind(long count) |
There was a problem hiding this comment.
It's the right trade-off for Rewind to be forcibly inlined?
There was a problem hiding this comment.
We believe so. It is only the fast path and is expected to be used frequently in some scenarios. I'll look to push the count check to the slow path like I did for advance to make this a bit better.
|
@dotnet-bot test OSX x64 Debug Build |
| <Compile Include="SequenceReader\OwnedArray.cs" /> | ||
| <Compile Include="SequenceReader\ReadTo.cs" /> | ||
| <Compile Include="SequenceReader\Rewind.cs" /> | ||
| <Compile Include="SequenceReader\SequenceSegment.cs" /> |
There was a problem hiding this comment.
nit: alphabetical ordering. swap SequenceSegment/SequenceFactory
|
👍🏿 |
* Add SequenceReader<T> Initial add of ReadOnlySequence<T> reader. Includes search and binary read functionality. Perf tests will be ported from CoreFXlab later. Parse functionality still being discussed. * Address feedback, fix non coreapp test builds. * Replace Peek() with TryCopyTo() * Address more feedback. Removing TryReadTo without advancePastDelmiter as we don't have the same pattern yet for the other overloads. * Rename files to not use underscore. * Fix uap builds * Move TryRead to SequenceMarshal and other feedback. * Tweak file name casing (part1) * Change casing pt 2. * Change package common types Commit migrated from dotnet/corefx@ab9570b
Initial add of
ReadOnlySequence<T>reader. Includes search and binary read functionality.Perf tests will be ported from CoreFXlab later. Text parsing functionality still being discussed and is not included in this change.
See https://github.com/dotnet/corefx/issues/32588.
cc: @joshfree, @danmosemsft, @davidfowl, @terrajobst, @ericstj (for packaging)