Tidying up - fix a bunch of TODOs and remove outdated ones.

csharp/src/Google.Protobuf.Test/CodedInputStreamTest.cs

@@ -281,7 +281,6 @@ namespace Google.Protobuf
             CodedInputStream input = new CodedInputStream(ms);
             Assert.AreEqual(tag, input.ReadTag());
 
-            // TODO(jonskeet): Should this be ArgumentNullException instead?
             Assert.Throws<InvalidProtocolBufferException>(() => input.ReadBytes());
         }
 

csharp/src/Google.Protobuf/ByteString.cs

@@ -40,7 +40,6 @@ namespace Google.Protobuf
 {
     /// <summary>
     /// Immutable array of bytes.
-    /// TODO(jonskeet): Implement the common collection interfaces?
     /// </summary>
     public sealed class ByteString : IEnumerable<byte>, IEquatable<ByteString>
     {
@@ -284,8 +283,6 @@ namespace Google.Protobuf
             return !(lhs == rhs);
         }
 
-        // TODO(jonskeet): CopyTo if it turns out to be required
-
         /// <summary>
         /// Compares this byte string with another object.
         /// </summary>

csharp/src/Google.Protobuf/CodedInputStream.cs

@@ -30,6 +30,7 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #endregion
 
+using Google.Protobuf.Collections;
 using System;
 using System.Collections.Generic;
 using System.IO;
@@ -40,20 +41,21 @@ namespace Google.Protobuf
     /// Readings and decodes protocol message fields.
     /// </summary>
     /// <remarks>
-    /// This class contains two kinds of methods:  methods that read specific
-    /// protocol message constructs and field types (e.g. ReadTag and
-    /// ReadInt32) and methods that read low-level values (e.g.
-    /// ReadRawVarint32 and ReadRawBytes).  If you are reading encoded protocol
-    /// messages, you should use the former methods, but if you are reading some
-    /// other format of your own design, use the latter. The names of the former
-    /// methods are taken from the protocol buffer type names, not .NET types.
-    /// (Hence ReadFloat instead of ReadSingle, and ReadBool instead of ReadBoolean.)
-    /// 
-    /// TODO(jonskeet): Consider whether recursion and size limits shouldn't be readonly,
-    /// set at construction time.
+    /// <para>
+    /// This class is generally used by generated code to read appropriate
+    /// primitives from the stream. It effectively encapsulates the lowest
+    /// levels of protocol buffer format.
+    /// </para>
+    /// <para>
+    /// Repeated fields and map fields are not handled by this class; use <see cref="RepeatedField{T}"/>
+    /// and <see cref="MapField{TKey, TValue}"/> to serialize such fields.
+    /// </para>
     /// </remarks>
     public sealed class CodedInputStream
     {
+        // TODO(jonskeet): Consider whether recursion and size limits shouldn't be readonly,
+        // set at construction time.
+
         /// <summary>
         /// Buffer of data read from the stream or provided at construction time.
         /// </summary>
@@ -1022,7 +1024,6 @@ namespace Google.Protobuf
             if (totalBytesRetired + bufferPos + size > currentLimit)
             {
                 // Read to the end of the stream (up to the current limit) anyway.
-                // TODO(jonskeet): This is the only usage of SkipRawBytes. Do we really need to do it?
                 SkipRawBytes(currentLimit - totalBytesRetired - bufferPos);
                 // Then fail.
                 throw InvalidProtocolBufferException.TruncatedMessage();

csharp/src/Google.Protobuf/CodedOutputStream.cs

@@ -30,6 +30,7 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #endregion
 
+using Google.Protobuf.Collections;
 using System;
 using System.IO;
 using System.Text;
@@ -40,14 +41,19 @@ namespace Google.Protobuf
     /// Encodes and writes protocol message fields.
     /// </summary>
     /// <remarks>
-    /// This class contains two kinds of methods:  methods that write specific
-    /// protocol message constructs and field types (e.g. WriteTag and
-    /// WriteInt32) and methods that write low-level values (e.g.
-    /// WriteRawVarint32 and WriteRawBytes).  If you are writing encoded protocol
-    /// messages, you should use the former methods, but if you are writing some
-    /// other format of your own design, use the latter. The names of the former
-    /// methods are taken from the protocol buffer type names, not .NET types.
-    /// (Hence WriteFloat instead of WriteSingle, and WriteBool instead of WriteBoolean.)
+    /// <para>
+    /// This class is generally used by generated code to write appropriate
+    /// primitives to the stream. It effectively encapsulates the lowest
+    /// levels of protocol buffer format. Unlike some other implementations,
+    /// this does not include combined "write tag and value" methods. Generated
+    /// code knows the exact byte representations of the tags they're going to write,
+    /// so there's no need to re-encode them each time. Manually-written code calling
+    /// this class should just call one of the <c>WriteTag</c> overloads before each value.
+    /// </para>
+    /// <para>
+    /// Repeated fields and map fields are not handled by this class; use <see cref="RepeatedField{T}"/>
+    /// and <see cref="MapField{TKey, TValue}"/> to serialize such fields.
+    /// </para>
     /// </remarks>
     public sealed partial class CodedOutputStream
     {

csharp/src/Google.Protobuf/Collections/RepeatedField.cs

@@ -88,7 +88,7 @@ namespace Google.Protobuf.Collections
         public void AddEntriesFrom(CodedInputStream input, FieldCodec<T> codec)
         {
             // TODO: Inline some of the Add code, so we can avoid checking the size on every
-            // iteration and the mutability.
+            // iteration.
             uint tag = input.LastTag;
             var reader = codec.ValueReader;
             // Value types can be packed or not.
@@ -315,7 +315,7 @@ namespace Google.Protobuf.Collections
             {
                 throw new ArgumentNullException("values");
             }
-            // TODO: Check for ICollection and get the Count?
+            // TODO: Check for ICollection and get the Count, to optimize?
             foreach (T item in values)
             {
                 Add(item);
@@ -394,7 +394,6 @@ namespace Google.Protobuf.Collections
             {
                 return false;
             }
-            // TODO(jonskeet): Does this box for enums?
             EqualityComparer<T> comparer = EqualityComparer<T>.Default;
             for (int i = 0; i < count; i++)
             {
@@ -418,7 +417,6 @@ namespace Google.Protobuf.Collections
             {
                 throw new ArgumentNullException("item");
             }
-            // TODO(jonskeet): Does this box for enums?
             EqualityComparer<T> comparer = EqualityComparer<T>.Default;
             for (int i = 0; i < count; i++)
             {

csharp/src/Google.Protobuf/FrameworkPortability.cs

@@ -40,10 +40,9 @@ namespace Google.Protobuf
     /// </summary>
     internal static class FrameworkPortability
     {
-        // TODO(jtattermusch): is this still a thing?
         // The value of RegexOptions.Compiled is 8. We can test for the presence at
         // execution time using Enum.IsDefined, so a single build will do the right thing
-        // on each platform.
+        // on each platform. (RegexOptions.Compiled isn't supported by PCLs.)
         internal static readonly RegexOptions CompiledRegexWhereAvailable =
             Enum.IsDefined(typeof(RegexOptions), 8) ? (RegexOptions)8 : RegexOptions.None;
     }

csharp/src/Google.Protobuf/IMessage.cs

@@ -35,8 +35,6 @@ using Google.Protobuf.Reflection;
 
 namespace Google.Protobuf
 {
-
-    // TODO(jonskeet): Do we want a "weak" (non-generic) version of IReflectedMessage?
     // TODO(jonskeet): Split these interfaces into separate files when we're happy with them.
 
     /// <summary>

csharp/src/Google.Protobuf/JsonFormatter.cs

@@ -158,7 +158,6 @@ namespace Google.Protobuf
             {
                 var accessor = field.Accessor;
                 // Oneofs are written later
-                // TODO: Change to write out fields in order, interleaving oneofs appropriately (as per binary format)
                 if (field.ContainingOneof != null && field.ContainingOneof.Accessor.GetCaseFieldDescriptor(message) != field)
                 {
                     continue;
@@ -425,7 +424,7 @@ namespace Google.Protobuf
             if (descriptor.FullName == ListValue.Descriptor.FullName)
             {
                 var fieldAccessor = descriptor.Fields[ListValue.ValuesFieldNumber].Accessor;
-                WriteList(builder, fieldAccessor, (IList) fieldAccessor.GetValue(value));
+                WriteList(builder, fieldAccessor, (IList) fieldAccessor.GetValue((IMessage) value));
                 return;
             }
             if (descriptor.FullName == Value.Descriptor.FullName)

csharp/src/Google.Protobuf/Reflection/FieldAccessorBase.cs

@@ -41,23 +41,23 @@ namespace Google.Protobuf.Reflection
     /// </summary>
     internal abstract class FieldAccessorBase : IFieldAccessor
     {
-        private readonly Func<object, object> getValueDelegate;
+        private readonly Func<IMessage, object> getValueDelegate;
         private readonly FieldDescriptor descriptor;
 
         internal FieldAccessorBase(PropertyInfo property, FieldDescriptor descriptor)
         {
             this.descriptor = descriptor;
-            getValueDelegate = ReflectionUtil.CreateFuncObjectObject(property.GetGetMethod());
+            getValueDelegate = ReflectionUtil.CreateFuncIMessageObject(property.GetGetMethod());
         }
 
         public FieldDescriptor Descriptor { get { return descriptor; } }
 
-        public object GetValue(object message)
+        public object GetValue(IMessage message)
         {
             return getValueDelegate(message);
         }
 
-        public abstract void Clear(object message);
-        public abstract void SetValue(object message, object value);
+        public abstract void Clear(IMessage message);
+        public abstract void SetValue(IMessage message, object value);
     }
 }

csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs

@@ -287,23 +287,23 @@ namespace Google.Protobuf.Reflection
             DescriptorPool pool = new DescriptorPool(dependencies);
             FileDescriptor result = new FileDescriptor(descriptorData, proto, dependencies, pool, allowUnknownDependencies, generatedCodeInfo);
 
-            // TODO(jonskeet): Reinstate these checks, or get rid of them entirely. They aren't in the Java code,
-            // and fail for the CustomOptions test right now. (We get "descriptor.proto" vs "google/protobuf/descriptor.proto".)
-            //if (dependencies.Length != proto.DependencyCount)
-            //{
-            //    throw new DescriptorValidationException(result,
-            //                                            "Dependencies passed to FileDescriptor.BuildFrom() don't match " +
-            //                                            "those listed in the FileDescriptorProto.");
-            //}
-            //for (int i = 0; i < proto.DependencyCount; i++)
-            //{
-            //    if (dependencies[i].Name != proto.DependencyList[i])
-            //    {
-            //        throw new DescriptorValidationException(result,
-            //                                                "Dependencies passed to FileDescriptor.BuildFrom() don't match " +
-            //                                                "those listed in the FileDescriptorProto.");
-            //    }
-            //}
+            // Validate that the dependencies we've been passed (as FileDescriptors) are actually the ones we
+            // need.
+            if (dependencies.Length != proto.Dependency.Count)
+            {
+                throw new DescriptorValidationException(result,
+                                                        "Dependencies passed to FileDescriptor.BuildFrom() don't match " +
+                                                        "those listed in the FileDescriptorProto.");
+            }
+            for (int i = 0; i < proto.Dependency.Count; i++)
+            {
+                if (dependencies[i].Name != proto.Dependency[i])
+                {
+                    throw new DescriptorValidationException(result,
+                                                            "Dependencies passed to FileDescriptor.BuildFrom() don't match " +
+                                                            "those listed in the FileDescriptorProto.");
+                }
+            }
 
             result.CrossLink();
             return result;

csharp/src/Google.Protobuf/Reflection/IFieldAccessor.cs

@@ -45,20 +45,20 @@ namespace Google.Protobuf.Reflection
         /// </summary>
         FieldDescriptor Descriptor { get; }
 
-        // TODO: Should the argument type for these messages be IReflectedMessage?
+        // TODO: Should the argument type for these messages be IMessage?
 
         /// <summary>
         /// Clears the field in the specified message. (For repeated fields,
         /// this clears the list.)
         /// </summary>
-        void Clear(object message);
+        void Clear(IMessage message);
 
         /// <summary>
         /// Fetches the field value. For repeated values, this will be an
         /// <see cref="IList"/> implementation. For map values, this will be an
         /// <see cref="IDictionary"/> implementation.
         /// </summary>
-        object GetValue(object message);
+        object GetValue(IMessage message);
 
         /// <summary>
         /// Mutator for single "simple" fields only.
@@ -68,6 +68,6 @@ namespace Google.Protobuf.Reflection
         /// Map fields are mutated by fetching the value and manipulating it as a dictionary.
         /// </remarks>
         /// <exception cref="InvalidOperationException">The field is not a "simple" field.</exception>
-        void SetValue(object message, object value);
+        void SetValue(IMessage message, object value);
     }
 }

csharp/src/Google.Protobuf/Reflection/MapFieldAccessor.cs

@@ -45,13 +45,13 @@ namespace Google.Protobuf.Reflection
         {
         }
 
-        public override void Clear(object message)
+        public override void Clear(IMessage message)
         {
             IDictionary list = (IDictionary) GetValue(message);
             list.Clear();
         }
 
-        public override void SetValue(object message, object value)
+        public override void SetValue(IMessage message, object value)
         {
             throw new InvalidOperationException("SetValue is not implemented for map fields");
         }

csharp/src/Google.Protobuf/Reflection/OneofAccessor.cs

@@ -41,8 +41,8 @@ namespace Google.Protobuf.Reflection
     /// </summary>
     public sealed class OneofAccessor
     {
-        private readonly Func<object, int> caseDelegate;
-        private readonly Action<object> clearDelegate;
+        private readonly Func<IMessage, int> caseDelegate;
+        private readonly Action<IMessage> clearDelegate;
         private OneofDescriptor descriptor;
 
         internal OneofAccessor(PropertyInfo caseProperty, MethodInfo clearMethod, OneofDescriptor descriptor) 
@@ -52,10 +52,10 @@ namespace Google.Protobuf.Reflection
                 throw new ArgumentException("Cannot read from property");
             }
             this.descriptor = descriptor;
-            caseDelegate = ReflectionUtil.CreateFuncObjectT<int>(caseProperty.GetGetMethod());
+            caseDelegate = ReflectionUtil.CreateFuncIMessageT<int>(caseProperty.GetGetMethod());
 
             this.descriptor = descriptor;
-            clearDelegate = ReflectionUtil.CreateActionObject(clearMethod);
+            clearDelegate = ReflectionUtil.CreateActionIMessage(clearMethod);
         }
 
         /// <summary>
@@ -69,7 +69,7 @@ namespace Google.Protobuf.Reflection
         /// <summary>
         /// Clears the oneof in the specified message.
         /// </summary>
-        public void Clear(object message)
+        public void Clear(IMessage message)
         {
             clearDelegate(message);
         }
@@ -77,7 +77,7 @@ namespace Google.Protobuf.Reflection
         /// <summary>
         /// Indicates which field in the oneof is set for specified message
         /// </summary>
-        public FieldDescriptor GetCaseFieldDescriptor(object message)
+        public FieldDescriptor GetCaseFieldDescriptor(IMessage message)
         {
             int fieldNumber = caseDelegate(message);
             if (fieldNumber > 0)

csharp/src/Google.Protobuf/Reflection/ReflectionUtil.cs

@@ -56,52 +56,52 @@ namespace Google.Protobuf.Reflection
         /// Creates a delegate which will cast the argument to the appropriate method target type,
         /// call the method on it, then convert the result to object.
         /// </summary>
-        internal static Func<object, object> CreateFuncObjectObject(MethodInfo method)
+        internal static Func<IMessage, object> CreateFuncIMessageObject(MethodInfo method)
         {
-            ParameterExpression parameter = Expression.Parameter(typeof(object), "p");
+            ParameterExpression parameter = Expression.Parameter(typeof(IMessage), "p");
             Expression downcast = Expression.Convert(parameter, method.DeclaringType);
             Expression call = Expression.Call(downcast, method);
             Expression upcast = Expression.Convert(call, typeof(object));
-            return Expression.Lambda<Func<object, object>>(upcast, parameter).Compile();
+            return Expression.Lambda<Func<IMessage, object>>(upcast, parameter).Compile();
         }
 
         /// <summary>
         /// Creates a delegate which will cast the argument to the appropriate method target type,
         /// call the method on it, then convert the result to the specified type.
         /// </summary>
-        internal static Func<object, T> CreateFuncObjectT<T>(MethodInfo method)
+        internal static Func<IMessage, T> CreateFuncIMessageT<T>(MethodInfo method)
         {
-            ParameterExpression parameter = Expression.Parameter(typeof(object), "p");
+            ParameterExpression parameter = Expression.Parameter(typeof(IMessage), "p");
             Expression downcast = Expression.Convert(parameter, method.DeclaringType);
             Expression call = Expression.Call(downcast, method);
             Expression upcast = Expression.Convert(call, typeof(T));
-            return Expression.Lambda<Func<object, T>>(upcast, parameter).Compile();
+            return Expression.Lambda<Func<IMessage, T>>(upcast, parameter).Compile();
         }
 
         /// <summary>
         /// Creates a delegate which will execute the given method after casting the first argument to
         /// the target type of the method, and the second argument to the first parameter type of the method.
         /// </summary>
-        internal static Action<object, object> CreateActionObjectObject(MethodInfo method)
+        internal static Action<IMessage, object> CreateActionIMessageObject(MethodInfo method)
         {
-            ParameterExpression targetParameter = Expression.Parameter(typeof(object), "target");
+            ParameterExpression targetParameter = Expression.Parameter(typeof(IMessage), "target");
             ParameterExpression argParameter = Expression.Parameter(typeof(object), "arg");
             Expression castTarget = Expression.Convert(targetParameter, method.DeclaringType);
             Expression castArgument = Expression.Convert(argParameter, method.GetParameters()[0].ParameterType);
             Expression call = Expression.Call(castTarget, method, castArgument);
-            return Expression.Lambda<Action<object, object>>(call, targetParameter, argParameter).Compile();
+            return Expression.Lambda<Action<IMessage, object>>(call, targetParameter, argParameter).Compile();
         }
 
         /// <summary>
         /// Creates a delegate which will execute the given method after casting the first argument to
         /// the target type of the method.
         /// </summary>
-        internal static Action<object> CreateActionObject(MethodInfo method)
+        internal static Action<IMessage> CreateActionIMessage(MethodInfo method)
         {
-            ParameterExpression targetParameter = Expression.Parameter(typeof(object), "target");
+            ParameterExpression targetParameter = Expression.Parameter(typeof(IMessage), "target");
             Expression castTarget = Expression.Convert(targetParameter, method.DeclaringType);
             Expression call = Expression.Call(castTarget, method);
-            return Expression.Lambda<Action<object>>(call, targetParameter).Compile();
+            return Expression.Lambda<Action<IMessage>>(call, targetParameter).Compile();
         }        
     }
 }

csharp/src/Google.Protobuf/Reflection/RepeatedFieldAccessor.cs

@@ -45,13 +45,13 @@ namespace Google.Protobuf.Reflection
         {
         }
 
-        public override void Clear(object message)
+        public override void Clear(IMessage message)
         {
             IList list = (IList) GetValue(message);
             list.Clear();
         }
 
-        public override void SetValue(object message, object value)
+        public override void SetValue(IMessage message, object value)
         {
             throw new InvalidOperationException("SetValue is not implemented for repeated fields");
         }

csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs

@@ -46,8 +46,8 @@ namespace Google.Protobuf.Reflection
         // and proto2 vs proto3 for non-message types, as proto3 doesn't support "full" presence detection or default
         // values.
 
-        private readonly Action<object, object> setValueDelegate;
-        private readonly Action<object> clearDelegate;
+        private readonly Action<IMessage, object> setValueDelegate;
+        private readonly Action<IMessage> clearDelegate;
 
         internal SingleFieldAccessor(PropertyInfo property, FieldDescriptor descriptor) : base(property, descriptor)
         {
@@ -55,12 +55,10 @@ namespace Google.Protobuf.Reflection
             {
                 throw new ArgumentException("Not all required properties/methods available");
             }
-            setValueDelegate = ReflectionUtil.CreateActionObjectObject(property.GetSetMethod());
+            setValueDelegate = ReflectionUtil.CreateActionIMessageObject(property.GetSetMethod());
 
             var clrType = property.PropertyType;
             
-            // TODO: What should clear on a oneof member do? Clear the oneof?
-
             // TODO: Validate that this is a reasonable single field? (Should be a value type, a message type, or string/ByteString.)
             object defaultValue =
                 typeof(IMessage).IsAssignableFrom(clrType) ? null
@@ -70,12 +68,12 @@ namespace Google.Protobuf.Reflection
             clearDelegate = message => SetValue(message, defaultValue);
         }
 
-        public override void Clear(object message)
+        public override void Clear(IMessage message)
         {
             clearDelegate(message);
         }
 
-        public override void SetValue(object message, object value)
+        public override void SetValue(IMessage message, object value)
         {
             setValueDelegate(message, value);
         }

csharp/src/Google.Protobuf/WireFormat.cs

@@ -43,19 +43,6 @@ namespace Google.Protobuf
     /// </summary>
     public static class WireFormat
     {
-        #region Fixed sizes.
-
-        // TODO(jonskeet): Move these somewhere else. They're messy. Consider making FieldType a smarter kind of enum
-        internal const int Fixed32Size = 4;
-        internal const int Fixed64Size = 8;
-        internal const int SFixed32Size = 4;
-        internal const int SFixed64Size = 8;
-        internal const int FloatSize = 4;
-        internal const int DoubleSize = 8;
-        internal const int BoolSize = 1;
-
-        #endregion
-
         /// <summary>
         /// Wire types within protobuf encoding.
         /// </summary>