-
Notifications
You must be signed in to change notification settings - Fork 184
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
Fix proto3 repeated field encoding without packed option #635
Conversation
proto3 repeated fields are packed by default, see https://developers.google.com/protocol-buffers/docs/encoding#packed. Fixes google#345
/// proto2 requires `[packed=true]` option. | ||
bool get isPacked { | ||
if (!isRepeated) { | ||
return false; // only repeated fields can be packed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this instead throw an argument-error (is it not an error to ask if a non-repeated field is packed?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The call site is this:
protobuf.dart/protoc_plugin/lib/src/protobuf_field.dart
Lines 157 to 169 in ad3f06f
String get typeConstant { | |
var prefix = 'O'; | |
if (isRequired) { | |
prefix = 'Q'; | |
} else if (isPacked) { | |
prefix = 'K'; | |
} else if (isRepeated) { | |
prefix = 'P'; | |
} | |
return '$protobufImportPrefix.PbFieldType.' + | |
prefix + | |
baseType.typeConstantSuffix; | |
} |
We don't check whether the field is repeated before checking whether it's packed. We could update the checks in the call site and expect repeated
field here. I don't have an opinion on whether that would be an improvement or not because there's only one use site. We could even inline this getter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the call-site it makes sense to just return false here I think.
I would prefer not inlining this amount of logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@sigurdm PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ported to internal in cl/449702819. |
proto3 repeated fields are packed by default, see
https://developers.google.com/protocol-buffers/docs/encoding#packed.
Fixes #345