From f262611ff65d5a5d1b4665e2d339f108ef29fcf9 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 10:13:56 +0000 Subject: 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. --- csharp/src/Google.Protobuf/FieldCodec.cs | 138 +++++++++++++++---------------- 1 file changed, 66 insertions(+), 72 deletions(-) (limited to 'csharp/src/Google.Protobuf/FieldCodec.cs') diff --git a/csharp/src/Google.Protobuf/FieldCodec.cs b/csharp/src/Google.Protobuf/FieldCodec.cs index c2b8d268..98313088 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 @@ namespace Google.Protobuf } /// + /// /// 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. + /// + /// + /// 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. + /// /// /// - /// 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. /// public sealed class FieldCodec { private static readonly T DefaultDefault; + private static readonly bool TypeSupportsPacking = typeof(T).IsValueType() && Nullable.GetUnderlyingType(typeof(T)) == null; static FieldCodec() { @@ -354,75 +362,31 @@ namespace Google.Protobuf // Otherwise it's the default value of the CLR type } - private readonly Func reader; - private readonly Action writer; - private readonly Func sizeCalculator; - private readonly uint tag; - 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 static bool IsPackedRepeatedField(uint tag) => + TypeSupportsPacking && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited; - internal FieldCodec( - Func reader, - Action writer, - Func sizeCalculator, - uint tag) : this(reader, writer, sizeCalculator, tag, DefaultDefault) - { - } - - internal FieldCodec( - Func reader, - Action writer, - Func sizeCalculator, - uint tag, - T defaultValue) - { - this.reader = reader; - this.writer = writer; - this.sizeCalculator = sizeCalculator; - this.fixedSize = 0; - this.tag = tag; - this.defaultValue = defaultValue; - tagSize = CodedOutputStream.ComputeRawVarint32Size(tag); - } - - internal FieldCodec( - Func reader, - Action writer, - int fixedSize, - uint tag) - { - this.reader = reader; - this.writer = writer; - this.sizeCalculator = _ => fixedSize; - this.fixedSize = fixedSize; - this.tag = tag; - tagSize = CodedOutputStream.ComputeRawVarint32Size(tag); - } + internal bool PackedRepeatedField { get; } /// - /// Returns the size calculator for just a value. + /// Returns a delegate to write a value (unconditionally) to a coded output stream. /// - internal Func ValueSizeCalculator { get { return sizeCalculator; } } + internal Action ValueWriter { get; } /// - /// Returns a delegate to write a value (unconditionally) to a coded output stream. + /// Returns the size calculator for just a value. /// - internal Action ValueWriter { get { return writer; } } + internal Func ValueSizeCalculator { get; } /// /// 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. /// - internal Func ValueReader { get { return reader; } } + internal Func ValueReader { get; } /// /// Returns the fixed size for an entry, or 0 if sizes vary. /// - internal int FixedSize { get { return fixedSize; } } + internal int FixedSize { get; } /// /// Gets the tag of the codec. @@ -430,15 +394,54 @@ namespace Google.Protobuf /// /// The tag of the codec. /// - public uint Tag { get { return tag; } } + internal uint Tag { get; } /// - /// Gets the default value of the codec's type. + /// 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. /// /// /// The default value of the codec's type. /// - public T DefaultValue { get { return defaultValue; } } + internal T DefaultValue { get; } + + private readonly int tagSize; + + internal FieldCodec( + Func reader, + Action writer, + int fixedSize, + uint tag) : this(reader, writer, _ => fixedSize, tag) + { + FixedSize = fixedSize; + } + + internal FieldCodec( + Func reader, + Action writer, + Func sizeCalculator, + uint tag) : this(reader, writer, sizeCalculator, tag, DefaultDefault) + { + } + + internal FieldCodec( + Func reader, + Action writer, + Func sizeCalculator, + uint tag, + T 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. + PackedRepeatedField = IsPackedRepeatedField(tag); + } /// /// Write a tag and the given value, *if* the value is not the default. @@ -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 /// /// The input stream to read from. /// The value read from the stream. - public T Read(CodedInputStream input) - { - return reader(input); - } + public T Read(CodedInputStream input) => ValueReader(input); /// /// Calculates the size required to write the given value, with a tag, /// if the value is not the default. /// - 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.Default.Equals(value, defaultValue); - } + private bool IsDefault(T value) => EqualityComparer.Default.Equals(value, DefaultValue); } } -- cgit v1.2.3