Fix handling of repeated wrappers
Previously we were incorrectly packing wrapper types.
This also refactors FieldCodec a bit as well, using more C# 6-ness.
diff --git a/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs b/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs
index b72ef98..a2c833f 100644
--- a/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs
+++ b/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs
@@ -149,6 +149,30 @@
}
[Test]
+ public void RepeatedWrappersBinaryFormat()
+ {
+ // At one point we accidentally used a packed format for repeated wrappers, which is wrong (and weird).
+ // This test is just to prove that we use the right format.
+
+ var rawOutput = new MemoryStream();
+ var output = new CodedOutputStream(rawOutput);
+ // Write a value of 5
+ output.WriteTag(RepeatedWellKnownTypes.Int32FieldFieldNumber, WireFormat.WireType.LengthDelimited);
+ output.WriteLength(2);
+ output.WriteTag(WrappersReflection.WrapperValueFieldNumber, WireFormat.WireType.Varint);
+ output.WriteInt32(5);
+ // Write a value of 0 (empty message)
+ output.WriteTag(RepeatedWellKnownTypes.Int32FieldFieldNumber, WireFormat.WireType.LengthDelimited);
+ output.WriteLength(0);
+ output.Flush();
+ var expectedBytes = rawOutput.ToArray();
+
+ var message = new RepeatedWellKnownTypes { Int32Field = { 5, 0 } };
+ var actualBytes = message.ToByteArray();
+ Assert.AreEqual(expectedBytes, actualBytes);
+ }
+
+ [Test]
public void MapWrappersSerializeDeserialize()
{
// Note: no null values here, as they are prohibited in map fields
diff --git a/csharp/src/Google.Protobuf/Collections/RepeatedField.cs b/csharp/src/Google.Protobuf/Collections/RepeatedField.cs
index e3f65af..1cde03b 100644
--- a/csharp/src/Google.Protobuf/Collections/RepeatedField.cs
+++ b/csharp/src/Google.Protobuf/Collections/RepeatedField.cs
@@ -34,7 +34,6 @@
using System.Collections;
using System.Collections.Generic;
using System.Text;
-using Google.Protobuf.Compatibility;
namespace Google.Protobuf.Collections
{
@@ -96,8 +95,8 @@
// iteration.
uint tag = input.LastTag;
var reader = codec.ValueReader;
- // Value types can be packed or not.
- if (typeof(T).IsValueType() && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited)
+ // Non-nullable value types can be packed or not.
+ if (FieldCodec<T>.IsPackedRepeatedField(tag))
{
int length = input.ReadLength();
if (length > 0)
@@ -134,7 +133,7 @@
return 0;
}
uint tag = codec.Tag;
- if (typeof(T).IsValueType() && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited)
+ if (codec.PackedRepeatedField)
{
int dataSize = CalculatePackedDataSize(codec);
return CodedOutputStream.ComputeRawVarint32Size(tag) +
@@ -186,7 +185,7 @@
}
var writer = codec.ValueWriter;
var tag = codec.Tag;
- if (typeof(T).IsValueType() && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited)
+ if (codec.PackedRepeatedField)
{
// Packed primitive type
uint size = (uint)CalculatePackedDataSize(codec);
diff --git a/csharp/src/Google.Protobuf/FieldCodec.cs b/csharp/src/Google.Protobuf/FieldCodec.cs
index c2b8d26..9831308 100644
--- a/csharp/src/Google.Protobuf/FieldCodec.cs
+++ b/csharp/src/Google.Protobuf/FieldCodec.cs
@@ -30,6 +30,7 @@
// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#endregion
+using Google.Protobuf.Compatibility;
using Google.Protobuf.WellKnownTypes;
using System;
using System.Collections.Generic;
@@ -329,17 +330,24 @@
}
/// <summary>
+ /// <para>
/// An encode/decode pair for a single field. This effectively encapsulates
/// all the information needed to read or write the field value from/to a coded
/// stream.
+ /// </para>
+ /// <para>
+ /// This class is public and has to be as it is used by generated code, but its public
+ /// API is very limited - just what the generated code needs to call directly.
+ /// </para>
/// </summary>
/// <remarks>
- /// This never writes default values to the stream, and is not currently designed
- /// to play well with packed arrays.
+ /// This never writes default values to the stream, and does not address "packedness"
+ /// in repeated fields itself, other than to know whether or not the field *should* be packed.
/// </remarks>
public sealed class FieldCodec<T>
{
private static readonly T DefaultDefault;
+ private static readonly bool TypeSupportsPacking = typeof(T).IsValueType() && Nullable.GetUnderlyingType(typeof(T)) == null;
static FieldCodec()
{
@@ -354,16 +362,60 @@
// Otherwise it's the default value of the CLR type
}
- private readonly Func<CodedInputStream, T> reader;
- private readonly Action<CodedOutputStream, T> writer;
- private readonly Func<T, int> sizeCalculator;
- private readonly uint tag;
+ internal static bool IsPackedRepeatedField(uint tag) =>
+ TypeSupportsPacking && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited;
+
+ internal bool PackedRepeatedField { get; }
+
+ /// <summary>
+ /// Returns a delegate to write a value (unconditionally) to a coded output stream.
+ /// </summary>
+ internal Action<CodedOutputStream, T> ValueWriter { get; }
+
+ /// <summary>
+ /// Returns the size calculator for just a value.
+ /// </summary>
+ internal Func<T, int> ValueSizeCalculator { get; }
+
+ /// <summary>
+ /// Returns a delegate to read a value from a coded input stream. It is assumed that
+ /// the stream is already positioned on the appropriate tag.
+ /// </summary>
+ internal Func<CodedInputStream, T> ValueReader { get; }
+
+ /// <summary>
+ /// Returns the fixed size for an entry, or 0 if sizes vary.
+ /// </summary>
+ internal int FixedSize { get; }
+
+ /// <summary>
+ /// Gets the tag of the codec.
+ /// </summary>
+ /// <value>
+ /// The tag of the codec.
+ /// </value>
+ internal uint Tag { get; }
+
+ /// <summary>
+ /// Default value for this codec. Usually the same for every instance of the same type, but
+ /// for string/ByteString wrapper fields the codec's default value is null, whereas for
+ /// other string/ByteString fields it's "" or ByteString.Empty.
+ /// </summary>
+ /// <value>
+ /// The default value of the codec's type.
+ /// </value>
+ internal T DefaultValue { get; }
+
private readonly int tagSize;
- private readonly int fixedSize;
- // Default value for this codec. Usually the same for every instance of the same type, but
- // for string/ByteString wrapper fields the codec's default value is null, whereas for
- // other string/ByteString fields it's "" or ByteString.Empty.
- private readonly T defaultValue;
+
+ internal FieldCodec(
+ Func<CodedInputStream, T> reader,
+ Action<CodedOutputStream, T> writer,
+ int fixedSize,
+ uint tag) : this(reader, writer, _ => fixedSize, tag)
+ {
+ FixedSize = fixedSize;
+ }
internal FieldCodec(
Func<CodedInputStream, T> reader,
@@ -380,66 +432,17 @@
uint tag,
T defaultValue)
{
- this.reader = reader;
- this.writer = writer;
- this.sizeCalculator = sizeCalculator;
- this.fixedSize = 0;
- this.tag = tag;
- this.defaultValue = defaultValue;
+ ValueReader = reader;
+ ValueWriter = writer;
+ ValueSizeCalculator = sizeCalculator;
+ FixedSize = 0;
+ Tag = tag;
+ DefaultValue = defaultValue;
tagSize = CodedOutputStream.ComputeRawVarint32Size(tag);
+ // Detect packed-ness once, so we can check for it within RepeatedField<T>.
+ PackedRepeatedField = IsPackedRepeatedField(tag);
}
- internal FieldCodec(
- Func<CodedInputStream, T> reader,
- Action<CodedOutputStream, T> writer,
- int fixedSize,
- uint tag)
- {
- this.reader = reader;
- this.writer = writer;
- this.sizeCalculator = _ => fixedSize;
- this.fixedSize = fixedSize;
- this.tag = tag;
- tagSize = CodedOutputStream.ComputeRawVarint32Size(tag);
- }
-
- /// <summary>
- /// Returns the size calculator for just a value.
- /// </summary>
- internal Func<T, int> ValueSizeCalculator { get { return sizeCalculator; } }
-
- /// <summary>
- /// Returns a delegate to write a value (unconditionally) to a coded output stream.
- /// </summary>
- internal Action<CodedOutputStream, T> ValueWriter { get { return writer; } }
-
- /// <summary>
- /// Returns a delegate to read a value from a coded input stream. It is assumed that
- /// the stream is already positioned on the appropriate tag.
- /// </summary>
- internal Func<CodedInputStream, T> ValueReader { get { return reader; } }
-
- /// <summary>
- /// Returns the fixed size for an entry, or 0 if sizes vary.
- /// </summary>
- internal int FixedSize { get { return fixedSize; } }
-
- /// <summary>
- /// Gets the tag of the codec.
- /// </summary>
- /// <value>
- /// The tag of the codec.
- /// </value>
- public uint Tag { get { return tag; } }
-
- /// <summary>
- /// Gets the default value of the codec's type.
- /// </summary>
- /// <value>
- /// The default value of the codec's type.
- /// </value>
- public T DefaultValue { get { return defaultValue; } }
-
/// <summary>
/// Write a tag and the given value, *if* the value is not the default.
/// </summary>
@@ -447,8 +450,8 @@
{
if (!IsDefault(value))
{
- output.WriteTag(tag);
- writer(output, value);
+ output.WriteTag(Tag);
+ ValueWriter(output, value);
}
}
@@ -457,23 +460,14 @@
/// </summary>
/// <param name="input">The input stream to read from.</param>
/// <returns>The value read from the stream.</returns>
- public T Read(CodedInputStream input)
- {
- return reader(input);
- }
+ public T Read(CodedInputStream input) => ValueReader(input);
/// <summary>
/// Calculates the size required to write the given value, with a tag,
/// if the value is not the default.
/// </summary>
- public int CalculateSizeWithTag(T value)
- {
- return IsDefault(value) ? 0 : sizeCalculator(value) + tagSize;
- }
+ public int CalculateSizeWithTag(T value) => IsDefault(value) ? 0 : ValueSizeCalculator(value) + tagSize;
- private bool IsDefault(T value)
- {
- return EqualityComparer<T>.Default.Equals(value, defaultValue);
- }
+ private bool IsDefault(T value) => EqualityComparer<T>.Default.Equals(value, DefaultValue);
}
}