C# Proto2 feature : Groups#5183
Conversation
|
I don't believe the failed ruby build is due to what I wrote, @anandolee. |
| uint oldTag = PushTag(); | ||
| ++recursionDepth; | ||
| builder.MergeFrom(this); | ||
| if (!ReachedTagLimit) |
There was a problem hiding this comment.
No need to push and pop the expected end group tag. Can calculate the expected tag and do the compare here directly.
| @@ -0,0 +1,188 @@ | |||
| // Protocol Buffers - Google's data interchange format | |||
There was a problem hiding this comment.
Both c++ and java do not have generators for group:
https://github.com/protocolbuffers/protobuf/tree/ba8692fbade4ba329cc4531e286ab5a8e0821d97/src/google/protobuf/compiler/java
https://github.com/protocolbuffers/protobuf/tree/ba8692fbade4ba329cc4531e286ab5a8e0821d97/src/google/protobuf/compiler/cpp
The group can share code gen with messages:
| // If we actually read a tag with a field of 0, that's not a valid tag. | ||
| throw InvalidProtocolBufferException.InvalidTag(); | ||
| } | ||
| if (ReachedLimit || ReachedTagLimit) |
There was a problem hiding this comment.
I understand you want to ReadTag returns 0 for the expected end group thus Merge/Parse can be stopped at the tag. However end group tag is a valid tag and returns 0 makes this function logically wrong. It will also introduce some error for example in SkipGroup():
It is easy to fix SkipGroup though but I still think ReadTag should return the valid tag. For Merge/Parse stop issue, I checked with java and it is using parseUnknownField() for decision :
anandolee
left a comment
anandolee
left a comment
There was a problem hiding this comment.
LGTM, just a little suggestion for better readability
| "if ($has_property_check$) {\n" | ||
| " subBuilder.MergeFrom($property_name$);\n" | ||
| "}\n" | ||
| "input.ReadMessage(subBuilder);\n" |
There was a problem hiding this comment.
Print the common code first and then do the type check (for better readability)
|
Fixed. I also include a fix for an issue that would prevent group end tags from being written in repeated fields. |
ObsidianMinor
force-pushed
the
csharp/proto2-feature/groups
branch
from
32d0c11 to
4156260
Compare
|
Fixed the other build files containing remnants of the separate group generators |
ObsidianMinor
force-pushed
the
csharp/proto2-feature/groups
branch
from
4156260 to
1f97065
Compare
|
I swear these build files are going to be the end of me. |
|
No worries, it is normal to fixing the build file several times :) |
4 participants
Another day, another proto2 PR for C#.
This PR adds support for groups (including unknown field support) in the C# library and code generator.