Skip to content

Commit

Permalink
Merge pull request #6431 from jtattermusch/backport_csharp_proto2_review
Browse files Browse the repository at this point in the history 
Backport review comments for C# proto2 features (for 3.9.x)
  • Loading branch information
@jtattermusch
jtattermusch committed Jul 25, 2019
2 parents 92f2c89 + b99e8f5 commit bc5f088
Show file tree
Hide file tree
Showing 4 changed files with 27 additions and 42 deletions.
16 changes: 9 additions & 7 deletions csharp/src/Google.Protobuf/Extension.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
namespace Google.Protobuf
{
/// <summary>
/// Represents a non-generic extension definition
/// Represents a non-generic extension definition. This API is experimental and subject to change.
/// </summary>
public abstract class Extension
{
Expand All @@ -44,9 +44,9 @@ public abstract class Extension
/// <summary>
/// Internal use. Creates a new extension with the specified field number.
/// </summary>
protected Extension(int number)
protected Extension(int fieldNumber)
{
FieldNumber = number;
FieldNumber = fieldNumber;
}

internal abstract IExtensionValue CreateValue();
Expand All @@ -58,7 +58,8 @@ protected Extension(int number)
}

/// <summary>
/// Represents a type-safe extension identifier used for getting and setting single extension values in <see cref="IExtendableMessage{T}"/> instances
/// Represents a type-safe extension identifier used for getting and setting single extension values in <see cref="IExtendableMessage{T}"/> instances.
/// This API is experimental and subject to change.
/// </summary>
/// <typeparam name="TTarget">The message type this field applies to</typeparam>
/// <typeparam name="TValue">The field value type of this extension</typeparam>
Expand All @@ -69,7 +70,7 @@ public sealed class Extension<TTarget, TValue> : Extension where TTarget : IExte
/// <summary>
/// Creates a new extension identifier with the specified field number and codec
/// </summary>
public Extension(int number, FieldCodec<TValue> codec) : base(number)
public Extension(int fieldNumber, FieldCodec<TValue> codec) : base(fieldNumber)
{
this.codec = codec;
}
Expand All @@ -85,7 +86,8 @@ internal override IExtensionValue CreateValue()
}

/// <summary>
/// Represents a type-safe extension identifier used for getting repeated extension values in <see cref="IExtendableMessage{T}"/> instances
/// Represents a type-safe extension identifier used for getting repeated extension values in <see cref="IExtendableMessage{T}"/> instances.
/// This API is experimental and subject to change.
/// </summary>
/// <typeparam name="TTarget">The message type this field applies to</typeparam>
/// <typeparam name="TValue">The repeated field value type of this extension</typeparam>
Expand All @@ -96,7 +98,7 @@ public sealed class RepeatedExtension<TTarget, TValue> : Extension where TTarget
/// <summary>
/// Creates a new repeated extension identifier with the specified field number and codec
/// </summary>
public RepeatedExtension(int number, FieldCodec<TValue> codec) : base(number)
public RepeatedExtension(int fieldNumber, FieldCodec<TValue> codec) : base(fieldNumber)
{
this.codec = codec;
}
Expand Down
32 changes: 11 additions & 21 deletions csharp/src/Google.Protobuf/ExtensionRegistry.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
namespace Google.Protobuf
{
/// <summary>
/// Provides extensions to messages while parsing
/// Provides extensions to messages while parsing. This API is experimental and subject to change.
/// </summary>
public sealed class ExtensionRegistry : ICollection<Extension>, IDeepCloneable<ExtensionRegistry>
{
Expand Down Expand Up @@ -67,9 +67,9 @@ private ExtensionRegistry(IDictionary<ObjectIntPair<Type>, Extension> collection
/// </summary>
bool ICollection<Extension>.IsReadOnly => false;

internal bool ContainsInputField(CodedInputStream stream, Type target, out Extension extension)
{
return extensions.TryGetValue(new ObjectIntPair<Type>(target, WireFormat.GetTagFieldNumber(stream.LastTag)), out extension);
internal bool ContainsInputField(CodedInputStream stream, Type target, out Extension extension)
{
return extensions.TryGetValue(new ObjectIntPair<Type>(target, WireFormat.GetTagFieldNumber(stream.LastTag)), out extension);
}

/// <summary>
Expand All @@ -82,24 +82,14 @@ public void Add(Extension extension)
extensions.Add(new ObjectIntPair<Type>(extension.TargetType, extension.FieldNumber), extension);
}

/// <summary>
/// Adds the specified extensions to the registry
/// </summary>
public void Add(params Extension[] newExtensions)
{
ProtoPreconditions.CheckNotNull(newExtensions, nameof(newExtensions));

Add((IEnumerable<Extension>)newExtensions);
}

/// <summary>
/// Adds the specified extensions to the reigstry
/// </summary>
public void Add(IEnumerable<Extension> newExtensions)
public void AddRange(IEnumerable<Extension> extensions)
{
ProtoPreconditions.CheckNotNull(newExtensions, nameof(newExtensions));
ProtoPreconditions.CheckNotNull(extensions, nameof(extensions));

foreach (var extension in newExtensions)
foreach (var extension in extensions)
Add(extension);
}

Expand Down Expand Up @@ -134,10 +124,10 @@ void ICollection<Extension>.CopyTo(Extension[] array, int arrayIndex)
if (array.Length - arrayIndex < Count)
throw new ArgumentException("The provided array is shorter than the number of elements in the registry");

for (int i = 0; i < array.Length; i++)
{
Extension extension = array[i];
extensions.Add(new ObjectIntPair<Type>(extension.TargetType, extension.FieldNumber), extension);
for (int i = 0; i < array.Length; i++)
{
Extension extension = array[i];
extensions.Add(new ObjectIntPair<Type>(extension.TargetType, extension.FieldNumber), extension);
}
}

Expand Down
19 changes: 6 additions & 13 deletions csharp/src/Google.Protobuf/ExtensionSet.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ namespace Google.Protobuf
/// <summary>
/// Methods for managing <see cref="ExtensionSet{TTarget}"/>s with null checking.
///
/// Most users will not use this class directly
/// Most users will not use this class directly and its API is experimental and subject to change.
/// </summary>
public static class ExtensionSet
{
private static bool GetValue<TTarget>(ref ExtensionSet<TTarget> set, Extension extension, out IExtensionValue value) where TTarget : IExtendableMessage<TTarget>
private static bool TryGetValue<TTarget>(ref ExtensionSet<TTarget> set, Extension extension, out IExtensionValue value) where TTarget : IExtendableMessage<TTarget>
{
if (set == null)
{
Expand All @@ -60,7 +60,7 @@ public static class ExtensionSet
public static TValue Get<TTarget, TValue>(ref ExtensionSet<TTarget> set, Extension<TTarget, TValue> extension) where TTarget : IExtendableMessage<TTarget>
{
IExtensionValue value;
if (GetValue(ref set, extension, out value))
if (TryGetValue(ref set, extension, out value))
{
return ((ExtensionValue<TValue>)value).GetValue();
}
Expand All @@ -76,7 +76,7 @@ public static class ExtensionSet
public static RepeatedField<TValue> Get<TTarget, TValue>(ref ExtensionSet<TTarget> set, RepeatedExtension<TTarget, TValue> extension) where TTarget : IExtendableMessage<TTarget>
{
IExtensionValue value;
if (GetValue(ref set, extension, out value))
if (TryGetValue(ref set, extension, out value))
{
return ((RepeatedExtensionValue<TValue>)value).GetValue();
}
Expand Down Expand Up @@ -111,7 +111,7 @@ public static class ExtensionSet
}

/// <summary>
/// Sets the value of the specified extension
/// Sets the value of the specified extension. This will make a new instance of ExtensionSet if the set is null.
/// </summary>
public static void Set<TTarget, TValue>(ref ExtensionSet<TTarget> set, Extension<TTarget, TValue> extension, TValue value) where TTarget : IExtendableMessage<TTarget>
{
Expand Down Expand Up @@ -140,14 +140,7 @@ public static class ExtensionSet
public static bool Has<TTarget, TValue>(ref ExtensionSet<TTarget> set, Extension<TTarget, TValue> extension) where TTarget : IExtendableMessage<TTarget>
{
IExtensionValue value;
if (GetValue(ref set, extension, out value))
{
return ((ExtensionValue<TValue>)value).HasValue;
}
else
{
return false;
}
return TryGetValue(ref set, extension, out value);
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ private void CrossLink()

private static void AddAllExtensions(FileDescriptor[] dependencies, GeneratedClrTypeInfo generatedInfo, ExtensionRegistry registry)
{
registry.Add(dependencies.SelectMany(GetAllDependedExtensions).Concat(GetAllGeneratedExtensions(generatedInfo)).ToArray());
registry.AddRange(dependencies.SelectMany(GetAllDependedExtensions).Concat(GetAllGeneratedExtensions(generatedInfo)).ToArray());
}

private static IEnumerable<Extension> GetAllGeneratedExtensions(GeneratedClrTypeInfo generated)
Expand Down

0 comments on commit bc5f088

Please sign in to comment.