organicmaps/protobuf
13
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity Actions

Fix handling of repeated wrappers

Browse source 
Previously we were incorrectly packing wrapper types.
This also refactors FieldCodec a bit as well, using more C# 6-ness.
This commit is contained in:
Jon Skeet 2016-01-15 10:13:56 +00:00
parent f2fe50bfc5
commit f262611ff6
3 changed files with 104 additions and 87 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

24
csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs
View file

@ -148,6 +148,30 @@ namespace Google.Protobuf.WellKnownTypes
Assert.AreEqual("Second", message.StringField[1]);
}
[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()
{

9
csharp/src/Google.Protobuf/Collections/RepeatedField.cs
View file

@ -34,7 +34,6 @@ using System;
using System.Collections;
using System.Collections.Generic;
using System.Text;
using Google.Protobuf.Compatibility;
namespace Google.Protobuf.Collections
{
@ -96,8 +95,8 @@ namespace Google.Protobuf.Collections
// 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 @@ namespace Google.Protobuf.Collections
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 @@ namespace Google.Protobuf.Collections
}
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);

158
csharp/src/Google.Protobuf/FieldCodec.cs
View file

@ -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 @@ namespace Google.Protobuf
}
/// <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 @@ namespace Google.Protobuf
// 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 @@ namespace Google.Protobuf
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 @@ namespace Google.Protobuf
{
if (!IsDefault(value))
{
output.WriteTag(tag);
writer(output, value);
output.WriteTag(Tag);
ValueWriter(output, value);
}
}
@ -457,23 +460,14 @@ namespace Google.Protobuf
/// </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);
}
}