-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add Span<T> Binary Reader/Writer APIs #24400
Changes from all commits
e304136
47d9d25
f8a360d
ce52219
2901bdd
89eee39
2fb65e0
3a57781
a4e981f
575f8cd
faa9d47
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| <CLSCompliant>false</CLSCompliant> | ||
| <DocumentationFile>$(OutputPath)$(MSBuildProjectName).xml</DocumentationFile> | ||
| <IsPartialFacadeAssembly Condition="'$(TargetGroup)' == 'netcoreapp' OR '$(TargetGroup)' == 'uap'">true</IsPartialFacadeAssembly> | ||
| <DefineConstants Condition="'$(IsPartialFacadeAssembly)' == 'true'">$(DefineConstants);IsPartialFacade</DefineConstants> | ||
| <DefineConstants Condition="'$(TargetGroup)'=='netcoreapp'">$(DefineConstants);netcoreapp</DefineConstants> | ||
| <DefineConstants Condition="'$(TargetGroup)'=='netstandard1.1'">$(DefineConstants);netstandard11</DefineConstants> | ||
| </PropertyGroup> | ||
|
|
@@ -24,6 +25,12 @@ | |
| <Compile Include="System\SpanExtensions.cs" /> | ||
| <Compile Include="System\SpanHelpers.T.cs" /> | ||
| <Compile Include="System\SpanHelpers.byte.cs" /> | ||
| <Compile Include="System\Buffers\Binary\Reader.cs" /> | ||
| <Compile Include="System\Buffers\Binary\ReaderBigEndian.cs" /> | ||
| <Compile Include="System\Buffers\Binary\ReaderLittleEndian.cs" /> | ||
| <Compile Include="System\Buffers\Binary\Writer.cs" /> | ||
| <Compile Include="System\Buffers\Binary\WriterBigEndian.cs" /> | ||
| <Compile Include="System\Buffers\Binary\WriterLittleEndian.cs" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(IsPartialFacadeAssembly)' != 'true'"> | ||
| <Compile Include="System\ReadOnlySpan.cs" /> | ||
|
|
@@ -46,10 +53,11 @@ | |
| <Reference Include="System.Reflection" /> | ||
| <Reference Include="System.Resources.ResourceManager" /> | ||
| <Reference Include="System.Runtime" /> | ||
| <Reference Include="System.Runtime.Extensions" /> | ||
| <Reference Include="System.Runtime.InteropServices" /> | ||
| <Reference Condition="'$(TargetGroup)' != 'netstandard1.1'" Include="System.Numerics.Vectors" /> | ||
| <Reference Condition="'$(TargetGroup)' != 'netstandard1.1'" Include="System.Runtime.CompilerServices.Unsafe" /> | ||
| <ProjectReference Condition="'$(TargetGroup)' == 'netstandard1.1'" Include="..\..\System.Runtime.CompilerServices.Unsafe\ref\System.Runtime.CompilerServices.Unsafe.csproj" /> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you have to change from ref to src here?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also why do we need this to be a ProjectReference at all? A normal reference should work.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From @safern, #24400 (comment)
For netstandard, there is a normal reference.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this. If we are building this library for netstandard1.1 then it cannot depend on a netstanard1.6 library. You might be getting away with this because of the ProjectReference but I still believe this should just be a Reference. |
||
| <ProjectReference Condition="'$(TargetGroup)' == 'netstandard1.1'" Include="..\..\System.Runtime.CompilerServices.Unsafe\src\System.Runtime.CompilerServices.Unsafe.ilproj" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(IsPartialFacadeAssembly)' == 'true'"> | ||
| <Compile Include="System\SpanExtensions.Fast.cs" /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,147 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.Runtime; | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| namespace System.Buffers.Binary | ||
| { | ||
| /// <summary> | ||
| /// Reads bytes as primitives with specific endianness | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// For native formats, SpanExtensions.Read<T> should be used. | ||
| /// Use these helpers when you need to read specific endinanness. | ||
| /// </remarks> | ||
| public static partial class BinaryPrimitives | ||
| { | ||
| /// <summary> | ||
| /// This is a no-op and added only for consistency. | ||
| /// This allows the caller to read a struct of numeric primitives and reverse each field | ||
| /// rather than having to skip sbyte fields. | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static sbyte ReverseEndianness(sbyte value) | ||
| { | ||
| return value; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Reverses a primitive value - performs an endianness swap | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static short ReverseEndianness(short value) | ||
| { | ||
| return (short)((value & 0x00FF) << 8 | (value & 0xFF00) >> 8); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Reverses a primitive value - performs an endianness swap | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static int ReverseEndianness(int value) => (int)ReverseEndianness((uint)value); | ||
|
|
||
| /// <summary> | ||
| /// Reverses a primitive value - performs an endianness swap | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static long ReverseEndianness(long value) => (long)ReverseEndianness((ulong)value); | ||
|
|
||
| /// <summary> | ||
| /// This is a no-op and added only for consistency. | ||
| /// This allows the caller to read a struct of numeric primitives and reverse each field | ||
| /// rather than having to skip byte fields. | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static byte ReverseEndianness(byte value) | ||
| { | ||
| return value; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Reverses a primitive value - performs an endianness swap | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static ushort ReverseEndianness(ushort value) | ||
| { | ||
| return (ushort)((value & 0x00FFU) << 8 | (value & 0xFF00U) >> 8); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Reverses a primitive value - performs an endianness swap | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static uint ReverseEndianness(uint value) | ||
| { | ||
| value = (value << 16) | (value >> 16); | ||
| value = (value & 0x00FF00FF) << 8 | (value & 0xFF00FF00) >> 8; | ||
| return value; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Reverses a primitive value - performs an endianness swap | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static ulong ReverseEndianness(ulong value) | ||
| { | ||
| value = (value << 32) | (value >> 32); | ||
| value = (value & 0x0000FFFF0000FFFF) << 16 | (value & 0xFFFF0000FFFF0000) >> 16; | ||
| value = (value & 0x00FF00FF00FF00FF) << 8 | (value & 0xFF00FF00FF00FF00) >> 8; | ||
| return value; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Reads a structure of type T out of a read-only span of bytes. | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static T ReadMachineEndian<T>(ReadOnlySpan<byte> buffer) | ||
| where T : struct | ||
| { | ||
| #if IsPartialFacade | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should always call SpanHelpers and have the #ifdef only in the one place which is in SpanHelpers.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We would still end up with the #ifdef for the ThrowHelper so it won't reduce the number of places we have to ifdef. I am not certain how to work around it (there is already an internal ThrowHelper class in mscorlib).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't the ThrowHelper call also be in the SpanHelper #ifdef? |
||
| if (RuntimeHelpers.IsReferenceOrContainsReferences<T>()) | ||
| { | ||
| throw new ArgumentException(SR.Format(SR.Argument_InvalidTypeWithPointersNotSupported, typeof(T))); | ||
| } | ||
| #else | ||
| if (SpanHelpers.IsReferenceOrContainsReferences<T>()) | ||
| { | ||
| ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T)); | ||
| } | ||
| #endif | ||
| if (Unsafe.SizeOf<T>() > buffer.Length) | ||
| { | ||
| throw new ArgumentOutOfRangeException(); | ||
| } | ||
| return Unsafe.ReadUnaligned<T>(ref buffer.DangerousGetPinnableReference()); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Reads a structure of type T out of a span of bytes. | ||
| /// <returns>If the span is too small to contain the type T, return false.</returns> | ||
| /// </summary> | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| public static bool TryReadMachineEndian<T>(ReadOnlySpan<byte> buffer, out T value) | ||
| where T : struct | ||
| { | ||
| #if IsPartialFacade | ||
| if (RuntimeHelpers.IsReferenceOrContainsReferences<T>()) | ||
| { | ||
| throw new ArgumentException(SR.Format(SR.Argument_InvalidTypeWithPointersNotSupported, typeof(T))); | ||
| } | ||
| #else | ||
| if (SpanHelpers.IsReferenceOrContainsReferences<T>()) | ||
| { | ||
| ThrowHelper.ThrowArgumentException_InvalidTypeWithPointersNotSupported(typeof(T)); | ||
| } | ||
| #endif | ||
| if (Unsafe.SizeOf<T>() > (uint)buffer.Length) | ||
| { | ||
| value = default; | ||
| return false; | ||
| } | ||
| value = Unsafe.ReadUnaligned<T>(ref buffer.DangerousGetPinnableReference()); | ||
| return true; | ||
| } | ||
| } | ||
| } | ||

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.
IsPartialFacade doesn't really seem to capture a real condition here. Can we instead switch the define to something that captures the thing you are trying to use? See https://github.com/dotnet/corefx/blob/master/Documentation/coding-guidelines/project-guidelines.md#define-naming-convention for our guidelines.
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 will try to resolve this issue in a separate PR given this one is already green. I could keep it the way I had before, which is separate netcoreapp and uap constants and do the 'or' in code.
So, replace
#if IsPartialFacadewith#if netcoreapp || uap. Does that work?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.
Or give it a FEATURE_NAME name specific to what you are trying to use.