Skip to content

Commit

Permalink
address code review feedback:
Browse files Browse the repository at this point in the history
- prefer NET over NETCOREAPP when writing #if defines
- add ConditionalTheory to make the SkipTestException actually work
- ObjectNullMultiple must not allow for 0 inputs
- preallocate the List<T> for TFMs other than .NET
  • Loading branch information
adamsitnik committed Jun 6, 2024
1 parent 489c66a commit 697cb0a
Show file tree
Hide file tree
Showing 13 changed files with 93 additions and 55 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ private protected ArrayRecord(ArrayInfo arrayInfo)
[RequiresDynamicCode("The code for an array of the specified type might not be available.")]
public Array GetArray(Type expectedArrayType, bool allowNulls = true)
{
#if NETCOREAPP
#if NET
ArgumentNullException.ThrowIfNull(expectedArrayType);
#else
if (expectedArrayType is null)
Expand Down Expand Up @@ -153,6 +153,7 @@ private protected ArrayRecord(ArrayInfo arrayInfo) : base(arrayInfo)
/// <see langword="true" /> to permit <see langword="null" /> values within the array;
/// otherwise, <see langword="false" />.
/// </param>
/// <returns>An array filled with the data provided in the serialized records.</returns>
public abstract T?[] GetArray(bool allowNulls = true);

#pragma warning disable IL3051 // RequiresDynamicCode is not required in this particualar case
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,39 +63,30 @@ internal static IReadOnlyList<T> DecodePrimitiveTypes(BinaryReader reader, int c
{
if (typeof(T) == typeof(byte) && reader.IsDataAvailable(count))
{
T[] result = (T[])(object)reader.ReadBytes(count);
// This might be less than the number of bytes requested if the end of the stream is reached.
if (result.Length != count)
{
ThrowHelper.ThrowEndOfStreamException();
}
return result;
return (T[])(object)reader.ReadBytes(count);
}
// the input is UTF8, so the check does not include sizeof(char)
else if (typeof(T) == typeof(char) && reader.IsDataAvailable(count))
{
T[] result = (T[])(object)reader.ReadChars(count);
if (result.Length != count)
{
ThrowHelper.ThrowEndOfStreamException();
}
return result;
return (T[])(object)reader.ReadChars(count);
}

// For decimals, the input is provided as strings, so we can't compute the required size up-front.
bool canPreAllocate = typeof(T) != typeof(decimal) && reader.IsDataAvailable(count * Unsafe.SizeOf<T>());
// Most of the tests use MemoryStream or FileStream and they both allow for executing the fast path.
// To ensure the slow path is tested as well, the fast path is executed only for optimized builds.
#if NET
if (typeof(T) != typeof(decimal) && typeof(T) != typeof(DateTime) && typeof(T) != typeof(TimeSpan) // not optimized
&& reader.IsDataAvailable(count * Unsafe.SizeOf<T>()))
#if NET && RELEASE
if (canPreAllocate)
{
return DecodePrimitiveTypesToArray(reader, count);
}
#endif
return DecodePrimitiveTypesToList(reader, count);
return DecodePrimitiveTypesToList(reader, count, canPreAllocate);
}

private static List<T> DecodePrimitiveTypesToList(BinaryReader reader, int count)
private static List<T> DecodePrimitiveTypesToList(BinaryReader reader, int count, bool canPreAllocate)
{
List<T> values = [];
List<T> values = new List<T>(canPreAllocate ? count : Math.Min(count, 4));
for (int i = 0; i < count; i++)
{
if (typeof(T) == typeof(byte))
Expand Down Expand Up @@ -174,35 +165,37 @@ private static T[] DecodePrimitiveTypesToArray(BinaryReader reader, int count)

if (!BitConverter.IsLittleEndian)
{
if (typeof(T) == typeof(short))
if (typeof(T) == typeof(short) || typeof(T) == typeof(ushort))
{
Span<short> span = MemoryMarshal.Cast<T, short>(result);
BinaryPrimitives.ReverseEndianness(span, span);
}
else if (typeof(T) == typeof(ushort))
{
Span<ushort> span = MemoryMarshal.Cast<T, ushort>(result);
BinaryPrimitives.ReverseEndianness(span, span);
}
else if (typeof(T) == typeof(int) || typeof(T) == typeof(float))
else if (typeof(T) == typeof(int) || typeof(T) == typeof(uint) || typeof(T) == typeof(float))
{
Span<int> span = MemoryMarshal.Cast<T, int>(result);
BinaryPrimitives.ReverseEndianness(span, span);
}
else if (typeof(T) == typeof(uint))
{
Span<uint> span = MemoryMarshal.Cast<T, uint>(result);
BinaryPrimitives.ReverseEndianness(span, span);
}
else if (typeof(T) == typeof(long) || typeof(T) == typeof(double))
else if (typeof(T) == typeof(long) || typeof(T) == typeof(ulong) || typeof(T) == typeof(double)
|| typeof(T) == typeof(DateTime) || typeof(T) == typeof(TimeSpan))
{
Span<long> span = MemoryMarshal.Cast<T, long>(result);
BinaryPrimitives.ReverseEndianness(span, span);
}
else if (typeof(T) == typeof(ulong))
{
Span<ulong> span = MemoryMarshal.Cast<T, ulong>(result);
BinaryPrimitives.ReverseEndianness(span, span);
}

if (typeof(T) == typeof(DateTime) || typeof(T) == typeof(TimeSpan))
{
Span<long> longs = MemoryMarshal.Cast<T, long>(result);
for (int i = 0; i < longs.Length; i++)
{
if (typeof(T) == typeof(DateTime))
{
result[i] = (T)(object)Utils.BinaryReaderExtensions.CreateDateTimeFromData(longs[i]);
}
else
{
result[i] = (T)(object)new TimeSpan(longs[i]);
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,10 @@ internal static ArrayRecord Decode(BinaryReader reader, RecordMap recordMap, Pay

bool isRectangular = arrayType is BinaryArrayType.Rectangular or BinaryArrayType.RectangularOffset;

if (rank < 1 || rank > 32
// It is an arbitrary limit in the current CoreCLR type loader.
const int MaxSupportedArrayRank = 32;

if (rank < 1 || rank > MaxSupportedArrayRank
|| (rank != 1 && !isRectangular)
|| (rank == 1 && isRectangular))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ internal static ClassInfo Decode(BinaryReader reader)
// however it's impossible to get such output with BinaryFormatter,
// so we prohibit that on purpose.
string memberName = reader.ReadString();
#if NETCOREAPP
#if NET
if (memberNames.TryAdd(memberName, i))
{
continue;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.IO;
using System.Runtime.Serialization.BinaryFormat.Utils;

namespace System.Runtime.Serialization.BinaryFormat;

Expand All @@ -20,5 +21,14 @@ internal sealed class ObjectNullMultiple256Record : NullsRecord
internal override int NullCount { get; }

internal static ObjectNullMultiple256Record Decode(BinaryReader reader)
=> new(reader.ReadByte());
{
// The NRBF spec for 2.5.6 ObjectNullMultiple allows for 0, but we don't.
byte count = reader.ReadByte();
if (count == 0)
{
ThrowHelper.ThrowInvalidValue(count);
}

return new ObjectNullMultiple256Record(count);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ internal sealed class ObjectNullMultipleRecord : NullsRecord
internal static ObjectNullMultipleRecord Decode(BinaryReader reader)
{
// 2.5.5 ObjectNullMultiple

// NullCount (4 bytes): An INT32 value ... The value MUST be a positive integer.
int count = reader.ReadInt32();
if (count <= 0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ static class PayloadReader
/// <returns><see langword="true" /> if it starts with NRBF payload header; otherwise, <see langword="false" />.</returns>
public static bool StartsWithPayloadHeader(byte[] bytes)
{
#if NETCOREAPP
#if NET
ArgumentNullException.ThrowIfNull(bytes);
#else
if (bytes is null)
Expand All @@ -37,7 +37,7 @@ public static bool StartsWithPayloadHeader(byte[] bytes)

return bytes.Length >= SerializedStreamHeaderRecord.Size
&& bytes[0] == (byte)RecordType.SerializedStreamHeader
#if NETCOREAPP
#if NET
&& BinaryPrimitives.ReadInt32LittleEndian(bytes.AsSpan(9)) == SerializedStreamHeaderRecord.MajorVersion
&& BinaryPrimitives.ReadInt32LittleEndian(bytes.AsSpan(13)) == SerializedStreamHeaderRecord.MinorVersion;
#else
Expand All @@ -58,7 +58,7 @@ public static bool StartsWithPayloadHeader(byte[] bytes)
/// <remarks><para>When this method returns, <paramref name="stream" /> will be restored to its original position.</para></remarks>
public static bool StartsWithPayloadHeader(Stream stream)
{
#if NETCOREAPP
#if NET
ArgumentNullException.ThrowIfNull(stream);
#else
if (stream is null)
Expand All @@ -81,7 +81,7 @@ public static bool StartsWithPayloadHeader(Stream stream)

try
{
#if NETCOREAPP
#if NET
stream.ReadExactly(buffer, 0, buffer.Length);
#else
int offset = 0;
Expand Down Expand Up @@ -134,7 +134,7 @@ public static SerializationRecord Read(Stream payload, PayloadOptions? options =
/// <inheritdoc cref="Read(Stream, PayloadOptions?, bool)"/>
public static SerializationRecord Read(Stream payload, out IReadOnlyDictionary<int, SerializationRecord> recordMap, PayloadOptions? options = default, bool leaveOpen = false)
{
#if NETCOREAPP
#if NET
ArgumentNullException.ThrowIfNull(payload);
#else
if (payload is null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace System.Runtime.Serialization.BinaryFormat;
/// <remarks>
/// <para>
/// The NRBF specification considers the following types to be primitive:
/// <see cref="String"/>, <see cref="bool"/>, <see cref="byte"/>, <see cref="sbyte"/>
/// <see cref="string"/>, <see cref="bool"/>, <see cref="byte"/>, <see cref="sbyte"/>
/// <see cref="char"/>, <see cref="short"/>, <see cref="ushort"/>,
/// <see cref="int"/>, <see cref="uint"/>, <see cref="long"/>, <see cref="ulong"/>,
/// <see cref="float"/>, <see cref="double"/>, <see cref="decimal"/>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ internal void Add(SerializationRecord record)
}
else
{
#if NETCOREAPP
#if NET
if (_map.TryAdd(record.ObjectId, record))
{
return;
Expand Down Expand Up @@ -84,7 +84,7 @@ private sealed class CollisionResistantInt32Comparer : IEqualityComparer<int>

public int GetHashCode(int obj)
{
#if NETCOREAPP
#if NET
Span<int> integers = new(ref obj);
#else
Span<int> integers = stackalloc int[1] { obj };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ enum RecordType : byte
/// Class information that references another class record's metadata.
/// </summary>
ClassWithId = 1,

// SystemClassWithMembers and ClassWithMembers are not supported by design (require type loading)

/// <summary>
/// A system class information with type info.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ public class EdgeCaseTests : ReadTests
[Fact]
public void SurrogatesGetNoSpecialHandling()
{
#if NETCOREAPP
#if NET
// Type is [Serializable] only on Full .NET Framework.
// So here we use a Base64 representation of serialized typeof(object)
const string serializedWithFullFramework = "AAEAAAD/////AQAAAAAAAAAEAQAAAB9TeXN0ZW0uVW5pdHlTZXJpYWxpemF0aW9uSG9sZGVyAwAAAAREYXRhCVVuaXR5VHlwZQxBc3NlbWJseU5hbWUBAAEIBgIAAAANU3lzdGVtLk9iamVjdAQAAAAGAwAAAEttc2NvcmxpYiwgVmVyc2lvbj00LjAuMC4wLCBDdWx0dXJlPW5ldXRyYWwsIFB1YmxpY0tleVRva2VuPWI3N2E1YzU2MTkzNGUwODkL";
Expand Down Expand Up @@ -60,11 +60,11 @@ public void ArraysOfStringsCanContainMemberReferences(FormatterTypeStyle typeFor
}
}

[Theory]
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))]
[InlineData(100)]
[InlineData(64_001)]
[InlineData(127_000)]
#if RELEASE && NETCOREAPP // it takes a lot of time to execute
#if RELEASE && NET // it takes a lot of time to execute
[InlineData(2147483591)] // Array.MaxLength
#endif
public void CanReadArrayOfAnySize(int length)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,37 @@ public void ThrowsWhenNumberOfNullsIsLargerThanArraySize(int nullCount, RecordTy
Assert.Throws<SerializationException>(() => PayloadReader.Read(stream));
}

[Theory]
[InlineData(0, RecordType.ObjectNullMultiple256)]
[InlineData(0, RecordType.ObjectNullMultiple)]
[InlineData(-1, RecordType.ObjectNullMultiple)]
public void ThrowsWhenNumberOfNullsIsInvalid(int nullCount, RecordType recordType)
{
using MemoryStream stream = new();
BinaryWriter writer = new(stream, Encoding.UTF8);

WriteSerializedStreamHeader(writer);

writer.Write((byte)RecordType.ArraySingleString);
writer.Write(1); // object ID
writer.Write(1); // length
writer.Write((byte)recordType);

if (recordType == RecordType.ObjectNullMultiple256)
{
writer.Write((byte)nullCount); // null count
}
else
{
writer.Write(nullCount); // null count
}

writer.Write((byte)RecordType.MessageEnd);

stream.Position = 0;
Assert.Throws<SerializationException>(() => PayloadReader.Read(stream));
}

[Fact]
public void ThrowWhenArrayOfStringsContainsReferenceToNonString()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ public void CanReadPrimitiveTypes()
Verify(decimal.MaxValue);
Verify(TimeSpan.MaxValue);
Verify(DateTime.Now);
#if NETCOREAPP
#if NET
Verify(nint.MaxValue);
Verify(nuint.MaxValue);
#endif
Expand Down

0 comments on commit 697cb0a

Please sign in to comment.