More TODOs done.

- Removed a TODO without change in DescriptorPool.LookupSymbol - the TODOs were around performance, and this is only used during descriptor initialization
- Make the CodedInputStream limits read-only, adding a static factory method for the rare cases when this is useful
- Extracted IDeepCloneable into its own file.

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

@@ -320,38 +320,18 @@ namespace Google.Protobuf
 
             Assert.Throws<InvalidProtocolBufferException>(() => TestRecursiveMessage.Parser.ParseFrom(data65));
 
-            CodedInputStream input = data64.CreateCodedInput();
-            input.SetRecursionLimit(8);
+            CodedInputStream input = CodedInputStream.CreateWithLimits(new MemoryStream(data64.ToByteArray()), 1000000, 63);
             Assert.Throws<InvalidProtocolBufferException>(() => TestRecursiveMessage.Parser.ParseFrom(input));
         }
 
-        /*
         [Test]
         public void SizeLimit()
         {
             // Have to use a Stream rather than ByteString.CreateCodedInput as SizeLimit doesn't
             // apply to the latter case.
-            MemoryStream ms = new MemoryStream(TestUtil.GetAllSet().ToByteString().ToByteArray());
-            CodedInputStream input = new CodedInputStream(ms);
-            input.SetSizeLimit(16);
-
-            Assert.Throws<InvalidProtocolBufferException>(() => TestAllTypes.ParseFrom(input));
-        }*/
-
-        [Test]
-        public void ResetSizeCounter()
-        {
-            CodedInputStream input = new CodedInputStream(
-                new SmallBlockInputStream(new byte[256], 8));
-            input.SetSizeLimit(16);
-            input.ReadRawBytes(16);
-
-            Assert.Throws<InvalidProtocolBufferException>(() => input.ReadRawByte());
-
-            input.ResetSizeCounter();
-            input.ReadRawByte(); // No exception thrown.
-
-            Assert.Throws<InvalidProtocolBufferException>(() => input.ReadRawBytes(16));
+            MemoryStream ms = new MemoryStream(SampleMessages.CreateFullTestAllTypes().ToByteArray());
+            CodedInputStream input = CodedInputStream.CreateWithLimits(ms, 16, 100);
+            Assert.Throws<InvalidProtocolBufferException>(() => TestAllTypes.Parser.ParseFrom(input));
         }
 
         /// <summary>
@@ -423,7 +403,7 @@ namespace Google.Protobuf
                 output.Flush();
 
                 ms.Position = 0;
-                CodedInputStream input = new CodedInputStream(ms, new byte[ms.Length / 2]);
+                CodedInputStream input = new CodedInputStream(ms, new byte[ms.Length / 2], 0, 0);
 
                 uint tag = input.ReadTag();
                 Assert.AreEqual(1, WireFormat.GetTagFieldNumber(tag));
@@ -528,5 +508,23 @@ namespace Google.Protobuf
             Assert.AreEqual(WireFormat.MakeTag(1, WireFormat.WireType.StartGroup), input.ReadTag());
             Assert.Throws<InvalidProtocolBufferException>(() => input.SkipLastField());
         }
+
+        [Test]
+        public void Construction_Invalid()
+        {
+            Assert.Throws<ArgumentNullException>(() => new CodedInputStream((byte[]) null));
+            Assert.Throws<ArgumentNullException>(() => new CodedInputStream(null, 0, 0));
+            Assert.Throws<ArgumentNullException>(() => new CodedInputStream((Stream) null));
+            Assert.Throws<ArgumentOutOfRangeException>(() => new CodedInputStream(new byte[10], 100, 0));
+            Assert.Throws<ArgumentOutOfRangeException>(() => new CodedInputStream(new byte[10], 5, 10));
+        }
+
+        [Test]
+        public void CreateWithLimits_InvalidLimits()
+        {
+            var stream = new MemoryStream();
+            Assert.Throws<ArgumentOutOfRangeException>(() => CodedInputStream.CreateWithLimits(stream, 0, 1));
+            Assert.Throws<ArgumentOutOfRangeException>(() => CodedInputStream.CreateWithLimits(stream, 1, 0));
+        }
     }
 }

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

@@ -334,7 +334,7 @@ namespace Google.Protobuf
             }
             // Now test Input stream:
             {
-                CodedInputStream cin = new CodedInputStream(new MemoryStream(bytes), new byte[50]);
+                CodedInputStream cin = new CodedInputStream(new MemoryStream(bytes), new byte[50], 0, 0);
                 Assert.AreEqual(0, cin.Position);
                 // Field 1:
                 uint tag = cin.ReadTag();

csharp/src/Google.Protobuf/CodedInputStream.cs

@@ -53,16 +53,13 @@ namespace Google.Protobuf
     /// </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>
         private readonly byte[] buffer;
 
         /// <summary>
-        /// The number of valid bytes in the buffer.
+        /// The index of the buffer at which we need to refill from the stream (if there is one).
         /// </summary>
         private int bufferSize;
 
@@ -106,61 +103,103 @@ namespace Google.Protobuf
         /// </summary> 
         private int currentLimit = int.MaxValue;
 
-        /// <summary>
-        /// <see cref="SetRecursionLimit"/>
-        /// </summary>
         private int recursionDepth = 0;
 
-        private int recursionLimit = DefaultRecursionLimit;
+        private readonly int recursionLimit;
+        private readonly int sizeLimit;
+
+        #region Construction
+        // Note that the checks are performed such that we don't end up checking obviously-valid things
+        // like non-null references for arrays we've just created.
 
         /// <summary>
-        /// <see cref="SetSizeLimit"/>
+        /// Creates a new CodedInputStream reading data from the given byte array.
         /// </summary>
-        private int sizeLimit = DefaultSizeLimit;
+        public CodedInputStream(byte[] buffer) : this(null, Preconditions.CheckNotNull(buffer, "buffer"), 0, buffer.Length)
+        {            
+        }
 
-        #region Construction
         /// <summary>
-        /// Creates a new CodedInputStream reading data from the given
-        /// byte array.
+        /// Creates a new CodedInputStream that reads from the given byte array slice.
         /// </summary>
-        public CodedInputStream(byte[] buf) : this(buf, 0, buf.Length)
-        {
+        public CodedInputStream(byte[] buffer, int offset, int length)
+            : this(null, Preconditions.CheckNotNull(buffer, "buffer"), offset, offset + length)
+        {            
+            if (offset < 0 || offset > buffer.Length)
+            {
+                throw new ArgumentOutOfRangeException("offset", "Offset must be within the buffer");
+            }
+            if (length < 0 || offset + length > buffer.Length)
+            {
+                throw new ArgumentOutOfRangeException("length", "Length must be non-negative and within the buffer");
+            }
         }
 
         /// <summary>
-        /// Creates a new CodedInputStream that reads from the given
-        /// byte array slice.
+        /// Creates a new CodedInputStream reading data from the given stream.
         /// </summary>
-        public CodedInputStream(byte[] buffer, int offset, int length)
+        public CodedInputStream(Stream input) : this(input, new byte[BufferSize], 0, 0)
         {
-            this.buffer = buffer;
-            this.bufferPos = offset;
-            this.bufferSize = offset + length;
-            this.input = null;
+            Preconditions.CheckNotNull(input, "input");
         }
 
         /// <summary>
-        /// Creates a new CodedInputStream reading data from the given stream.
+        /// Creates a new CodedInputStream reading data from the given
+        /// stream and buffer, using the default limits.
         /// </summary>
-        public CodedInputStream(Stream input)
+        internal CodedInputStream(Stream input, byte[] buffer, int bufferPos, int bufferSize)
         {
-            this.buffer = new byte[BufferSize];
-            this.bufferSize = 0;
             this.input = input;
+            this.buffer = buffer;
+            this.bufferPos = bufferPos;
+            this.bufferSize = bufferSize;
+            this.sizeLimit = DefaultSizeLimit;
+            this.recursionLimit = DefaultRecursionLimit;
         }
 
         /// <summary>
         /// Creates a new CodedInputStream reading data from the given
-        /// stream, with a pre-allocated buffer.
+        /// stream and buffer, using the specified limits.
         /// </summary>
-        internal CodedInputStream(Stream input, byte[] buffer)
+        /// <remarks>
+        /// This chains to the version with the default limits instead of vice versa to avoid
+        /// having to check that the default values are valid every time.
+        /// </remarks>
+        internal CodedInputStream(Stream input, byte[] buffer, int bufferPos, int bufferSize, int sizeLimit, int recursionLimit)
+            : this(input, buffer, bufferPos, bufferSize)
         {
-            this.buffer = buffer;
-            this.bufferSize = 0;
-            this.input = input;
+            if (sizeLimit <= 0)
+            {
+                throw new ArgumentOutOfRangeException("sizeLimit", "Size limit must be positive");
+            }
+            if (recursionLimit <= 0)
+            {
+                throw new ArgumentOutOfRangeException("recursionLimit!", "Recursion limit must be positive");
+            }
+            this.sizeLimit = sizeLimit;
+            this.recursionLimit = recursionLimit;
         }
         #endregion
 
+        /// <summary>
+        /// Creates a <see cref="CodedInputStream"/> with the specified size and recursion limits, reading
+        /// from an input stream.
+        /// </summary>
+        /// <remarks>
+        /// This method exists separately from the constructor to reduce the number of constructor overloads.
+        /// It is likely to be used considerably less frequently than the constructors, as the default limits
+        /// are suitable for most use cases.
+        /// </remarks>
+        /// <param name="input">The input stream to read from</param>
+        /// <param name="sizeLimit">The total limit of data to read from the stream.</param>
+        /// <param name="recursionLimit">The maximum recursion depth to allow while reading.</param>
+        /// <returns>A <c>CodedInputStream</c> reading from <paramref name="input"/> with the specified size
+        /// and recursion limits.</returns>
+        public static CodedInputStream CreateWithLimits(Stream input, int sizeLimit, int recursionLimit)
+        {
+            return new CodedInputStream(input, new byte[BufferSize], 0, 0, sizeLimit, recursionLimit);
+        }
+
         /// <summary>
         /// Returns the current position in the input stream, or the position in the input buffer
         /// </summary>
@@ -182,59 +221,30 @@ namespace Google.Protobuf
         /// </summary>
         internal uint LastTag { get { return lastTag; } }
 
-        #region Limits for recursion and length
         /// <summary>
-        /// Set the maximum message recursion depth.
+        /// Returns the size limit for this stream.
         /// </summary>
         /// <remarks>
-        /// In order to prevent malicious
-        /// messages from causing stack overflows, CodedInputStream limits
-        /// how deeply messages may be nested.  The default limit is 64.
+        /// This limit is applied when reading from the underlying stream, as a sanity check. It is
+        /// not applied when reading from a byte array data source without an underlying stream.
+        /// The default value is 64MB.
         /// </remarks>
-        public int SetRecursionLimit(int limit)
-        {
-            if (limit < 0)
-            {
-                throw new ArgumentOutOfRangeException("Recursion limit cannot be negative: " + limit);
-            }
-            int oldLimit = recursionLimit;
-            recursionLimit = limit;
-            return oldLimit;
-        }
+        /// <value>
+        /// The size limit.
+        /// </value>
+        public int SizeLimit { get { return sizeLimit; } }
 
         /// <summary>
-        /// Set the maximum message size.
+        /// Returns the recursion limit for this stream. This limit is applied whilst reading messages,
+        /// to avoid maliciously-recursive data.
         /// </summary>
         /// <remarks>
-        /// In order to prevent malicious messages from exhausting memory or
-        /// causing integer overflows, CodedInputStream limits how large a message may be.
-        /// The default limit is 64MB.  You should set this limit as small
-        /// as you can without harming your app's functionality.  Note that
-        /// size limits only apply when reading from an InputStream, not
-        /// when constructed around a raw byte array (nor with ByteString.NewCodedInput).
-        /// If you want to read several messages from a single CodedInputStream, you
-        /// can call ResetSizeCounter() after each message to avoid hitting the
-        /// size limit.
+        /// The default limit is 64.
         /// </remarks>
-        public int SetSizeLimit(int limit)
-        {
-            if (limit < 0)
-            {
-                throw new ArgumentOutOfRangeException("Size limit cannot be negative: " + limit);
-            }
-            int oldLimit = sizeLimit;
-            sizeLimit = limit;
-            return oldLimit;
-        }
-
-        /// <summary>
-        /// Resets the current size counter to zero (see <see cref="SetSizeLimit"/>).
-        /// </summary>
-        public void ResetSizeCounter()
-        {
-            totalBytesRetired = 0;
-        }
-        #endregion
+        /// <value>
+        /// The recursion limit for this stream.
+        /// </value>
+        public int RecursionLimit { get { return recursionLimit; } }
 
         #region Validation
         /// <summary>

csharp/src/Google.Protobuf/Google.Protobuf.csproj

@@ -83,6 +83,7 @@
     <Compile Include="Compatibility\TypeExtensions.cs" />
     <Compile Include="FieldCodec.cs" />
     <Compile Include="FrameworkPortability.cs" />
+    <Compile Include="IDeepCloneable.cs" />
     <Compile Include="JsonFormatter.cs" />
     <Compile Include="MessageExtensions.cs" />
     <Compile Include="IMessage.cs" />

csharp/src/Google.Protobuf/IDeepCloneable.cs

@@ -0,0 +1,54 @@
+#region Copyright notice and license
+// Protocol Buffers - Google's data interchange format
+// Copyright 2015 Google Inc.  All rights reserved.
+// https://developers.google.com/protocol-buffers/
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#endregion
+
+namespace Google.Protobuf
+{
+    /// <summary>
+    /// Generic interface for a deeply cloneable type.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// All generated messages implement this interface, but so do some non-message types.
+    /// Additionally, due to the type constraint on <c>T</c> in <see cref="IMessage{T}"/>,
+    /// it is simpler to keep this as a separate interface.
+    /// </para>
+    /// </remarks>
+    /// <typeparam name="T">The type itself, returned by the <see cref="Clone"/> method.</typeparam>
+    public interface IDeepCloneable<T>
+    {
+        /// <summary>
+        /// Creates a deep clone of this object.
+        /// </summary>
+        /// <returns>A deep clone of this object.</returns>
+        T Clone();
+    }
+}

csharp/src/Google.Protobuf/IMessage.cs

@@ -35,8 +35,6 @@ using Google.Protobuf.Reflection;
 
 namespace Google.Protobuf
 {
-    // TODO(jonskeet): Split these interfaces into separate files when we're happy with them.
-
     /// <summary>
     /// Interface for a Protocol Buffers message, supporting
     /// basic operations required for serialization.
@@ -86,24 +84,4 @@ namespace Google.Protobuf
         /// <param name="message">The message to merge with this one. Must not be null.</param>
         void MergeFrom(T message);
     }
-
-    /// <summary>
-    /// Generic interface for a deeply cloneable type.
-    /// </summary>
-    /// <remarks>
-    /// <para>
-    /// All generated messages implement this interface, but so do some non-message types.
-    /// Additionally, due to the type constraint on <c>T</c> in <see cref="IMessage{T}"/>,
-    /// it is simpler to keep this as a separate interface.
-    /// </para>
-    /// </remarks>
-    /// <typeparam name="T">The type itself, returned by the <see cref="Clone"/> method.</typeparam>
-    public interface IDeepCloneable<T>
-    {
-        /// <summary>
-        /// Creates a deep clone of this object.
-        /// </summary>
-        /// <returns>A deep clone of this object.</returns>
-        T Clone();
-    }
 }

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

@@ -257,10 +257,12 @@ namespace Google.Protobuf.Reflection
         /// or unqualified. C++-like name lookup semantics are used to search for the
         /// matching descriptor.
         /// </summary>
+        /// <remarks>
+        /// This isn't heavily optimized, but it's only used during cross linking anyway.
+        /// If it starts being used more widely, we should look at performance more carefully.
+        /// </remarks>
         internal IDescriptor LookupSymbol(string name, IDescriptor relativeTo)
         {
-            // TODO(jonskeet):  This could be optimized in a number of ways.
-
             IDescriptor result;
             if (name.StartsWith("."))
             {
@@ -282,7 +284,6 @@ namespace Google.Protobuf.Reflection
                 {
                     // Chop off the last component of the scope.
 
-                    // TODO(jonskeet): Make this more efficient. May not be worth using StringBuilder at all
                     int dotpos = scopeToTry.ToString().LastIndexOf(".");
                     if (dotpos == -1)
                     {