More tests for CodedInputStream, and some more WireFormat

committer: Jon Skeet <skeet@pobox.com>

5 files changed, 140 insertions, 116 deletions

diff --git a/csharp/ProtocolBuffers/CodedInputStream.cs b/csharp/ProtocolBuffers/CodedInputStream.cs
index a7b52197..c16596b1 100644
--- a/csharp/ProtocolBuffers/CodedInputStream.cs
+++ b/csharp/ProtocolBuffers/CodedInputStream.cs
@@ -37,11 +37,11 @@ namespace Google.ProtocolBuffers {
   /// set at construction time.
   /// </remarks>
   public sealed class CodedInputStream {
-    private byte[] buffer;
+    private readonly byte[] buffer;
     private int bufferSize;
     private int bufferSizeAfterLimit = 0;
     private int bufferPos = 0;
-    private Stream input;
+    private readonly Stream input;
     private uint lastTag = 0;
 
     const int DefaultRecursionLimit = 64;
@@ -102,14 +102,6 @@ namespace Google.ProtocolBuffers {
     #endregion
 
     #region Uncategorised (TODO: Fix this!)
-      /*
-   * Verifies that the last call to readTag() returned the given tag value.
-   * This is used to verify that a nested group ended with the correct
-   * end tag.
-   *
-   * @throws InvalidProtocolBufferException {@code value} does not match the
-   *                                        last tag.
-   */
     /// <summary>
     /// Verifies that the last call to ReadTag() returned the given tag value.
     /// This is used to verify that a nested group ended with the correct
@@ -131,7 +123,7 @@ namespace Google.ProtocolBuffers {
     /// since a protocol message may legally end wherever a tag occurs, and
     /// zero is not a valid tag number.
     /// </summary>
-    public int ReadTag() {
+    public uint ReadTag() {
       if (bufferPos == bufferSize && !RefillBuffer(false)) {
         lastTag = 0;
         return 0;
@@ -142,7 +134,7 @@ namespace Google.ProtocolBuffers {
         // If we actually read zero, that's not a valid tag.
         throw InvalidProtocolBufferException.InvalidTag();
       }
-      return (int) lastTag;
+      return lastTag;
     }
 
     /// <summary>
@@ -324,16 +316,10 @@ namespace Google.ProtocolBuffers {
       return DecodeZigZag64(ReadRawVarint64());
     }
 
-    /**
-     * Read a field of any primitive type.  Enums, groups, and embedded
-     * messages are not handled by this method.
-     *
-     * @param type Declared type of the field.
-     * @return An object representing the field's value, of the exact
-     *         type which would be returned by
-     *         {@link Message#getField(Descriptors.FieldDescriptor)} for
-     *         this field.
-     */
+    /// <summary>
+    /// Reads a field of any primitive type. Enums, groups and embedded
+    /// messages are not handled by this method.
+    /// </summary>
     public object readPrimitiveField(Descriptors.FieldDescriptor.Type fieldType) {
       switch (fieldType) {
         case Descriptors.FieldDescriptor.Type.Double:   return ReadDouble();
@@ -372,35 +358,35 @@ namespace Google.ProtocolBuffers {
     /// </summary>
     /// <returns></returns>
     public uint ReadRawVarint32() {
-      uint tmp = ReadRawByte();
-      if (tmp >= 0) {
-        return tmp;
+      int tmp = ReadRawByte();
+      if (tmp < 128) {
+        return (uint) tmp;
       }
-      uint result = tmp & 0x7f;
-      if ((tmp =ReadRawByte()) >= 0) {
+      int result = tmp & 0x7f;
+      if ((tmp = ReadRawByte()) < 128) {
         result |= tmp << 7;
       } else {
         result |= (tmp & 0x7f) << 7;
-        if ((tmp = ReadRawByte()) >= 0) {
+        if ((tmp = ReadRawByte()) < 128) {
           result |= tmp << 14;
         } else {
           result |= (tmp & 0x7f) << 14;
-          if ((tmp = ReadRawByte()) >= 0) {
+          if ((tmp = ReadRawByte()) < 128) {
             result |= tmp << 21;
           } else {
             result |= (tmp & 0x7f) << 21;
             result |= (tmp = ReadRawByte()) << 28;
-            if (tmp < 0) {
+            if (tmp >= 128) {
               // Discard upper 32 bits.
               for (int i = 0; i < 5; i++) {
-                if (ReadRawByte() >= 0) return result;
+                if (ReadRawByte() < 128) return (uint) result;
               }
               throw InvalidProtocolBufferException.MalformedVarint();
             }
           }
         }
       }
-      return result;
+      return (uint) result;
     }
 
     /// <summary>
@@ -443,7 +429,7 @@ namespace Google.ProtocolBuffers {
       long b6 = ReadRawByte();
       long b7 = ReadRawByte();
       long b8 = ReadRawByte();
-      return b1 | (b2 << 8) | (b3 << 8) | (b4 << 8)
+      return b1 | (b2 << 8) | (b3 << 16) | (b4 << 24)
           | (b5 << 32) | (b6 << 40) | (b7 << 48) | (b8 << 56);
     }
     #endregion
@@ -689,9 +675,8 @@ namespace Google.ProtocolBuffers {
           byte[] chunk = new byte[Math.Min(sizeLeft, BufferSize)];
           int pos = 0;
           while (pos < chunk.Length) {
-            int n = (input == null) ? -1 :
-              input.Read(chunk, pos, chunk.Length - pos);
-            if (n == -1) {
+            int n = (input == null) ? -1 : input.Read(chunk, pos, chunk.Length - pos);
+            if (n <= 0) {
               throw InvalidProtocolBufferException.TruncatedMessage();
             }
             totalBytesRetired += n;
diff --git a/csharp/ProtocolBuffers/IBuilder.cs b/csharp/ProtocolBuffers/IBuilder.cs
index 312cf842..bf4f46ff 100644
--- a/csharp/ProtocolBuffers/IBuilder.cs
+++ b/csharp/ProtocolBuffers/IBuilder.cs
@@ -1,12 +1,63 @@
 using System;
 using System.Collections.Generic;
-using System.Text;
 using System.IO;
 
 namespace Google.ProtocolBuffers {
 
+  /// <summary>
+  /// Non-generic interface for all members whose signatures don't require knowledge of
+  /// the type being built. The generic interface extends this one. Some methods return
+  /// either an IBuilder or an IMessage; in these cases the generic interface redeclares
+  /// the same method with a type-specific signature. Implementations are encouraged to
+  /// use explicit interface implemenation for the non-generic form. This mirrors
+  /// how IEnumerable and IEnumerable&lt;T&gt; work.
+  /// </summary>
   public interface IBuilder {
-    void MergeFrom(CodedInputStream codedInputStream, ExtensionRegistry extensionRegistry);
+    IBuilder MergeFrom(CodedInputStream codedInputStream, ExtensionRegistry extensionRegistry);
+    /// <summary>
+    /// Returns true iff all required fields in the message and all
+    /// embedded messages are set.
+    /// </summary>
+    bool Initialized { get; }
+
+    /// <summary>
+    /// Behaves like the equivalent property in IMessage&lt;T&gt;.
+    /// The returned map may or may not reflect future changes to the builder.
+    /// Either way, the returned map is unmodifiable.
+    /// </summary>
+    IDictionary<ProtocolBuffers.Descriptors.FieldDescriptor, object> AllFields { get; }
+
+    /// <summary>
+    /// Allows getting and setting of a field.
+    /// <see cref="IMessage{T}.Item(Descriptors.FieldDescriptor)"/>
+    /// </summary>
+    /// <param name="field"></param>
+    /// <returns></returns>
+    object this[Descriptors.FieldDescriptor field] { get; set; }
+
+    /// <summary>
+    /// Get the message's type's descriptor.
+    /// <see cref="IMessage{T}.DescriptorForType"/>
+    /// </summary>
+    Descriptors.Descriptor DescriptorForType { get; }
+
+    /// <summary>
+    /// <see cref="IMessage{T}.GetRepeatedFieldCount"/>
+    /// </summary>
+    /// <param name="field"></param>
+    /// <returns></returns>
+    int GetRepeatedFieldCount(Descriptors.FieldDescriptor field);
+
+    /// <summary>
+    /// Allows getting and setting of a repeated field value.
+    /// <see cref="IMessage{T}.Item(Descriptors.FieldDescriptor, int)"/>
+    /// </summary>
+    object this[Descriptors.FieldDescriptor field, int index] { get; set; }
+
+    /// <summary>
+    /// <see cref="IMessage{T}.HasField"/>
+    /// </summary>
+    bool HasField(Descriptors.FieldDescriptor field);
   }
 
   /// <summary>
@@ -14,7 +65,7 @@ namespace Google.ProtocolBuffers {
   /// TODO(jonskeet): Consider "SetXXX" methods returning the builder, as well as the properties.
   /// </summary>
   /// <typeparam name="T">Type of message</typeparam>
-  public interface IBuilder<T> where T : IMessage<T> {
+  public interface IBuilder<T> : IBuilder where T : IMessage<T> {
     /// <summary>
     /// Resets all fields to their default values.
     /// </summary>
@@ -60,12 +111,6 @@ namespace Google.ProtocolBuffers {
     IBuilder<T> Clone();
 
     /// <summary>
-    /// Returns true iff all required fields in the message and all
-    /// embedded messages are set.
-    /// </summary>
-    bool Initialized { get; }
-
-    /// <summary>
     /// Parses a message of this type from the input and merges it with this
     /// message, as if using MergeFrom(IMessage&lt;T&gt;).
     /// </summary>
@@ -92,13 +137,7 @@ namespace Google.ProtocolBuffers {
     /// in <paramref name="extensionRegistry"/>. Extensions not in the registry
     /// will be treated as unknown fields.
     /// </summary>
-    IBuilder<T> MergeFrom(CodedInputStream input, ExtensionRegistry extensionRegistry);
-
-    /// <summary>
-    /// Get the message's type's descriptor.
-    /// <see cref="IMessage{T}.DescriptorForType"/>
-    /// </summary>
-    Descriptors.Descriptor DescriptorForType { get; }
+    new IBuilder<T> MergeFrom(CodedInputStream input, ExtensionRegistry extensionRegistry);
 
     /// <summary>
     /// Get's the message's type's default instance.
@@ -107,13 +146,6 @@ namespace Google.ProtocolBuffers {
     IMessage<T> DefaultInstanceForType { get; }
 
     /// <summary>
-    /// Behaves like the equivalent property in IMessage&lt;T&gt;.
-    /// The returned map may or may not reflect future changes to the builder.
-    /// Either way, the returned map is unmodifiable.
-    /// </summary>
-    IDictionary<ProtocolBuffers.Descriptors.FieldDescriptor, object> AllFields { get; }
-
-    /// <summary>
     /// Create a builder for messages of the appropriate type for the given field.
     /// Messages built with this can then be passed to the various mutation properties
     /// and methods.
@@ -121,21 +153,7 @@ namespace Google.ProtocolBuffers {
     /// <typeparam name="TField"></typeparam>
     /// <param name="field"></param>
     /// <returns></returns>
-    IBuilder<TField> NewBuilderForField<TField>(Descriptors.FieldDescriptor field)
-        where TField : IMessage<TField>;
-
-    /// <summary>
-    /// <see cref="IMessage{T}.HasField"/>
-    /// </summary>
-    bool HasField(Descriptors.FieldDescriptor field);
-
-    /// <summary>
-    /// Allows getting and setting of a field.
-    /// <see cref="IMessage{T}.Item(Descriptors.FieldDescriptor)"/>
-    /// </summary>
-    /// <param name="field"></param>
-    /// <returns></returns>
-    object this[Descriptors.FieldDescriptor field] { get; set; }
+    IBuilder<TField> NewBuilderForField<TField>(Descriptors.FieldDescriptor field) where TField : IMessage<TField>;
 
     /// <summary>
     /// Clears the field. This is exactly equivalent to calling the generated
@@ -146,20 +164,6 @@ namespace Google.ProtocolBuffers {
     IBuilder<T> ClearField(Descriptors.FieldDescriptor field);
 
     /// <summary>
-    /// <see cref="IMessage{T}.GetRepeatedFieldCount"/>
-    /// </summary>
-    /// <param name="field"></param>
-    /// <returns></returns>
-    int GetRepeatedFieldCount(Descriptors.FieldDescriptor field);
-
-
-    /// <summary>
-    /// Allows getting and setting of a repeated field value.
-    /// <see cref="IMessage{T}.Item(Descriptors.FieldDescriptor, int)"/>
-    /// </summary>
-    object this[Descriptors.FieldDescriptor field, int index] { get; set; }
-
-    /// <summary>
     /// Appends the given value as a new element for the specified repeated field.
     /// </summary>
     /// <exception cref="ArgumentException">the field is not a repeated field,
diff --git a/csharp/ProtocolBuffers/IMessage.cs b/csharp/ProtocolBuffers/IMessage.cs
index 6a41d78e..109b2bfd 100644
--- a/csharp/ProtocolBuffers/IMessage.cs
+++ b/csharp/ProtocolBuffers/IMessage.cs
@@ -5,18 +5,13 @@ using System.IO;
 namespace Google.ProtocolBuffers {
 
   /// <summary>
-  /// Non-generic interface.
-  /// TODO(jonskeet): Do we want or need this?
+  /// Non-generic interface implemented by all Protocol Buffers messages.
+  /// Some members are repeated in the generic interface but with a
+  /// type-specific signature. Type-safe implementations
+  /// are encouraged to implement these non-generic members explicitly,
+  /// and the generic members implicitly.
   /// </summary>
   public interface IMessage {
-    void WriteTo(CodedOutputStream output);
-    int SerializedSize { get; }
-  }
-
-  /// <summary>
-  /// Interface implemented by all Protocol Buffers messages.
-  /// </summary>
-  public interface IMessage<T> where T : IMessage<T> {
     /// <summary>
     /// Returns the message's type's descriptor. This differs from the
     /// Descriptor property of each generated message class in that this
@@ -24,16 +19,6 @@ namespace Google.ProtocolBuffers {
     /// a static property of a specific class. They return the same thing.
     /// </summary>
     Descriptors.Descriptor DescriptorForType { get; }
-
-    /// <summary>
-    /// Returns an instance of this message type with all fields set to
-    /// their default values. This may or may not be a singleton. This differs
-    /// from the DefaultInstance property of each generated message class in that this
-    /// method is an abstract method of IMessage whereas DefaultInstance is
-    /// a static property of a specific class. They return the same thing.
-    /// </summary>
-    IMessage<T> DefaultInstanceForType { get; }
-
     /// <summary>
     /// Returns a collection of all the fields in this message which are set
     /// and their corresponding values.  A singular ("required" or "optional")
@@ -151,12 +136,42 @@ namespace Google.ProtocolBuffers {
     void WriteTo(Stream output);
     #endregion
 
-    #region Builders
+    #region Weakly typed members
+    /// <summary>
+    /// Returns an instance of this message type with all fields set to
+    /// their default values. This may or may not be a singleton. This differs
+    /// from the DefaultInstance property of each generated message class in that this
+    /// method is an abstract method of IMessage whereas DefaultInstance is
+    /// a static property of a specific class. They return the same thing.
+    /// </summary>
+    IMessage DefaultInstanceForType { get; }
+
     /// <summary>
     /// Constructs a new builder for a message of the same type as this message.
     /// </summary>
-    IBuilder<T> NewBuilderForType();
+    IBuilder NewBuilderForType();
     #endregion
+  }
+
+  /// <summary>
+  /// Type-safe interface for all generated messages to implement.
+  /// </summary>
+  public interface IMessage<T> : IMessage where T : IMessage<T> {
+    /// <summary>
+    /// Returns an instance of this message type with all fields set to
+    /// their default values. This may or may not be a singleton. This differs
+    /// from the DefaultInstance property of each generated message class in that this
+    /// method is an abstract method of IMessage whereas DefaultInstance is
+    /// a static property of a specific class. They return the same thing.
+    /// </summary>
+    new IMessage<T> DefaultInstanceForType { get; }
+
 
+    #region Builders
+    /// <summary>
+    /// Constructs a new builder for a message of the same type as this message.
+    /// </summary>
+    new IBuilder<T> NewBuilderForType();
+    #endregion
   }
 }
diff --git a/csharp/ProtocolBuffers/InvalidProtocolBufferException.cs b/csharp/ProtocolBuffers/InvalidProtocolBufferException.cs
index bf898722..db16ca8f 100644
--- a/csharp/ProtocolBuffers/InvalidProtocolBufferException.cs
+++ b/csharp/ProtocolBuffers/InvalidProtocolBufferException.cs
@@ -14,7 +14,8 @@ namespace Google.ProtocolBuffers {
       : base(message) {
     }
 
-    internal static InvalidProtocolBufferException TruncatedMessage() {
+    /// TODO(jonskeet): Make this internal again and use InternalVisibleTo?
+    public static InvalidProtocolBufferException TruncatedMessage() {
       return new InvalidProtocolBufferException(
         "While parsing a protocol message, the input ended unexpectedly " +
         "in the middle of a field.  This could mean either than the " +
@@ -22,13 +23,14 @@ namespace Google.ProtocolBuffers {
         "misreported its own length.");
     }
 
+    /// TODO(jonskeet): Make this internal again and use InternalVisibleTo?
     internal static InvalidProtocolBufferException NegativeSize() {
       return new InvalidProtocolBufferException(
         "CodedInputStream encountered an embedded string or message " +
         "which claimed to have negative size.");
     }
 
-    internal static InvalidProtocolBufferException MalformedVarint() {
+    public static InvalidProtocolBufferException MalformedVarint() {
       return new InvalidProtocolBufferException(
         "CodedInputStream encountered a malformed varint.");
     }
diff --git a/csharp/ProtocolBuffers/WireFormat.cs b/csharp/ProtocolBuffers/WireFormat.cs
index df15526a..b100e520 100644
--- a/csharp/ProtocolBuffers/WireFormat.cs
+++ b/csharp/ProtocolBuffers/WireFormat.cs
@@ -4,7 +4,7 @@ using System.Text;
 
 namespace Google.ProtocolBuffers {
   public class WireFormat {
-    public enum WireType {
+    public enum WireType : uint {
       Varint = 0,
       Fixed64 = 1,
       LengthDelimited = 2,
@@ -19,10 +19,28 @@ namespace Google.ProtocolBuffers {
       internal const int Message = 3;
     }
 
-    public static uint MakeTag(int fieldNumber, WireType type) {
+    private const int TagTypeBits = 3;
+    private const uint TagTypeMask = (1 << TagTypeBits) - 1;
 
-      // FIXME
-      return 0;
+    /// <summary>
+    /// Given a tag value, determines the wire type (lower 3 bits).
+    /// </summary>
+    public static WireType GetTagWireType(uint tag) {
+      return (WireType) (tag & TagTypeMask);
+    }
+
+    /// <summary>
+    /// Given a tag value, determines the field number (the upper 29 bits).
+    /// </summary>
+    public static uint GetTagFieldNumber(uint tag) {
+      return tag >> TagTypeBits;
+    }
+
+    /// <summary>
+    /// Makes a tag value given a field number and wire type.
+    /// </summary>
+    public static uint MakeTag(int fieldNumber, WireType wireType) {
+      return (uint) (fieldNumber << TagTypeBits) | (uint) wireType;
     }
   }
 }