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.

1 files changed, 66 insertions, 72 deletions

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
     }
 
     /// <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,75 +362,31 @@ 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;
-        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<CodedInputStream, T> reader,
-            Action<CodedOutputStream, T> writer,
-            Func<T, int> sizeCalculator,
-            uint tag) : this(reader, writer, sizeCalculator, tag, DefaultDefault)
-        {
-        }
-
-        internal FieldCodec(
-            Func<CodedInputStream, T> reader,
-            Action<CodedOutputStream, T> writer,
-            Func<T, int> 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<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);
-        }
+        internal bool PackedRepeatedField { get; }
 
         /// <summary>
-        /// Returns the size calculator for just a value.
+        /// Returns a delegate to write a value (unconditionally) to a coded output stream.
         /// </summary>
-        internal Func<T, int> ValueSizeCalculator { get { return sizeCalculator; } }
+        internal Action<CodedOutputStream, T> ValueWriter { get; }
 
         /// <summary>
-        /// Returns a delegate to write a value (unconditionally) to a coded output stream.
+        /// Returns the size calculator for just a value.
         /// </summary>
-        internal Action<CodedOutputStream, T> ValueWriter { get { return writer; } }
+        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 { return reader; } }
+        internal Func<CodedInputStream, T> ValueReader { get; }
 
         /// <summary>
         /// Returns the fixed size for an entry, or 0 if sizes vary.
         /// </summary>
-        internal int FixedSize { get { return fixedSize; } }
+        internal int FixedSize { get; }
 
         /// <summary>
         /// Gets the tag of the codec.
@@ -430,15 +394,54 @@ namespace Google.Protobuf
         /// <value>
         /// The tag of the codec.
         /// </value>
-        public uint Tag { get { return tag; } }
+        internal uint Tag { get; }
 
         /// <summary>
-        /// 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.
         /// </summary>
         /// <value>
         /// The default value of the codec's type.
         /// </value>
-        public T DefaultValue { get { return defaultValue; } }
+        internal T DefaultValue { get; }
+
+        private readonly int tagSize;
+        
+        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,
+            Action<CodedOutputStream, T> writer,
+            Func<T, int> sizeCalculator,
+            uint tag) : this(reader, writer, sizeCalculator, tag, DefaultDefault)
+        {
+        }
+
+        internal FieldCodec(
+            Func<CodedInputStream, T> reader,
+            Action<CodedOutputStream, T> writer,
+            Func<T, int> 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<T>.
+            PackedRepeatedField = IsPackedRepeatedField(tag);
+        }
 
         /// <summary>
         /// 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
         /// </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);
     }
 }