Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup various bits of Google.Protobuf #6674

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 10 additions & 0 deletions csharp/src/Google.Protobuf.Test/GeneratedMessageTest.Proto2.cs
Expand Up @@ -261,6 +261,16 @@ public void RequiredFields()
Assert.True(message.IsInitialized());
}

// Code was accidentally left in message parser that threw exceptions when missing required fields after parsing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding the comment.
nit: Use /// <summary> block for comments outside the method body.

// We've decided to not throw exceptions on missing fields, instead leaving it up to the consumer how they
// want to check and handle missing fields.
[Test]
public void RequiredFieldsNoThrow()
{
Assert.DoesNotThrow(() => TestRequired.Parser.ParseFrom(new byte[0]));
Assert.DoesNotThrow(() => (TestRequired.Parser as MessageParser).ParseFrom(new byte[0]));
}

[Test]
public void RequiredFieldsInExtensions()
{
Expand Down
2 changes: 1 addition & 1 deletion csharp/src/Google.Protobuf/CodedOutputStream.cs
Expand Up @@ -89,7 +89,7 @@ public CodedOutputStream(byte[] flatArray) : this(flatArray, 0, flatArray.Length
private CodedOutputStream(byte[] buffer, int offset, int length)
{
this.output = null;
this.buffer = buffer;
this.buffer = ProtoPreconditions.CheckNotNull(buffer, nameof(buffer));
this.position = offset;
this.limit = offset + length;
leaveOpen = true; // Simple way of avoiding trying to dispose of a null reference
Expand Down
2 changes: 1 addition & 1 deletion csharp/src/Google.Protobuf/JsonFormatter.cs
Expand Up @@ -133,7 +133,7 @@ static JsonFormatter()
/// <param name="settings">The settings.</param>
public JsonFormatter(Settings settings)
{
this.settings = settings;
this.settings = ProtoPreconditions.CheckNotNull(settings, nameof(settings));
}

/// <summary>
Expand Down
2 changes: 1 addition & 1 deletion csharp/src/Google.Protobuf/JsonParser.cs
Expand Up @@ -110,7 +110,7 @@ private static void MergeWrapperField(JsonParser parser, IMessage message, JsonT
/// <param name="settings">The settings.</param>
public JsonParser(Settings settings)
{
this.settings = settings;
this.settings = ProtoPreconditions.CheckNotNull(settings, nameof(settings));
}

/// <summary>
Expand Down
12 changes: 0 additions & 12 deletions csharp/src/Google.Protobuf/MessageParser.cs
Expand Up @@ -72,7 +72,6 @@ public IMessage ParseFrom(byte[] data)
{
IMessage message = factory();
message.MergeFrom(data, DiscardUnknownFields, Extensions);
CheckMergedRequiredFields(message);
ObsidianMinor marked this conversation as resolved.
Show resolved Hide resolved
return message;
}

Expand All @@ -87,7 +86,6 @@ public IMessage ParseFrom(byte[] data, int offset, int length)
{
IMessage message = factory();
message.MergeFrom(data, offset, length, DiscardUnknownFields, Extensions);
CheckMergedRequiredFields(message);
return message;
}

Expand All @@ -100,7 +98,6 @@ public IMessage ParseFrom(ByteString data)
{
IMessage message = factory();
message.MergeFrom(data, DiscardUnknownFields, Extensions);
CheckMergedRequiredFields(message);
return message;
}

Expand All @@ -113,7 +110,6 @@ public IMessage ParseFrom(Stream input)
{
IMessage message = factory();
message.MergeFrom(input, DiscardUnknownFields, Extensions);
CheckMergedRequiredFields(message);
return message;
}

Expand All @@ -130,7 +126,6 @@ public IMessage ParseDelimitedFrom(Stream input)
{
IMessage message = factory();
message.MergeDelimitedFrom(input, DiscardUnknownFields, Extensions);
CheckMergedRequiredFields(message);
return message;
}

Expand All @@ -143,7 +138,6 @@ public IMessage ParseFrom(CodedInputStream input)
{
IMessage message = factory();
MergeFrom(message, input);
CheckMergedRequiredFields(message);
return message;
}

Expand Down Expand Up @@ -176,12 +170,6 @@ internal void MergeFrom(IMessage message, CodedInputStream codedInput)
}
}

internal static void CheckMergedRequiredFields(IMessage message)
{
if (!message.IsInitialized())
throw new InvalidOperationException("Parsed message does not contain all required fields");
}

/// <summary>
/// Creates a new message parser which optionally discards unknown fields when parsing.
/// </summary>
Expand Down
9 changes: 5 additions & 4 deletions csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs
Expand Up @@ -107,13 +107,14 @@ internal void CrossLink()
{
descriptor.CrossLink();

IList<FieldDescriptor> _;
if (!declarationOrder.TryGetValue(descriptor.ExtendeeType, out _))
IList<FieldDescriptor> list;
if (!declarationOrder.TryGetValue(descriptor.ExtendeeType, out list))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • in protobuf codebase, always use curly brackets for if statements (single statements ifs and loops are considered dangerous).
  • I think
list = new List<FieldDescriptor>();
declarationOrder.Add(......., list);

is more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add an editorconfig with csharp_prefer_braces enabled to prevent style issues like this and others in the future? In another PR of course.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point, I'd like to add a set of rules to enforce code style in the protobuf C# codebase. I'm not sure what the best approach is (there are multiple ways of doing this).

{
declarationOrder.Add(descriptor.ExtendeeType, new List<FieldDescriptor>());
list = new List<FieldDescriptor>();
declarationOrder.Add(descriptor.ExtendeeType, list);
}

declarationOrder[descriptor.ExtendeeType].Add(descriptor);
list.Add(descriptor);
}

extensionsByTypeInDeclarationOrder = declarationOrder
Expand Down