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

Proto3 repeated field not packed by default #197

Closed
cygery opened this issue Jun 21, 2016 · 3 comments
Closed

Proto3 repeated field not packed by default #197

cygery opened this issue Jun 21, 2016 · 3 comments

Comments

@cygery
Copy link

cygery commented Jun 21, 2016

Message definitions

proto3_test.proto:

syntax = "proto3";

package test;

message Test3Int32RepDefault {
    repeated int32 repeated_int32 = 1;
}

message Test3Int32RepPackedTrue {
    repeated int32 repeated_int32 = 1 [packed = true];
}

Compiled via:

$ protoc --version
libprotoc 3.0.0
$ protoc --go_out=. *.proto

Result

proto3_test.pb.go:

// ...
type Test3Int32RepDefault struct {
    RepeatedInt32 []int32 `protobuf:"varint,1,rep,name=repeated_int32,json=repeatedInt32" json:"repeated_int32,omitempty"`
}
// ...
type Test3Int32RepPackedTrue struct {
    RepeatedInt32 []int32 `protobuf:"varint,1,rep,packed,name=repeated_int32,json=repeatedInt32" json:"repeated_int32,omitempty"`
}
// ...

The repeated field with default packed setting compiles to a non-packed field although it should be packed by default.

@edgesite
Copy link

The bugs seems points to

if field.Options != nil && field.Options.GetPacked() {

As Proto3 specified, repeated fields of scalar numeric types use packed encoding by default. We should add some check here.

** This is an urgent bug because the official Protobuf Javascript implementation doesn't read tag to detect binary is packed or not. **

Both declared proto3 syntax.
repeated int64:
image

repeated int64 [packed=false]:
image

@cygery
Copy link
Author

cygery commented Jul 20, 2016

Thanks for your comment. Yeah, I've also submitted an issue regarding the JavaScript parser which was labelled as a bug.
For now I'm explicitly defining the packed option as a workaround but I hope both issues will be fixed eventually.

@edgesite
Copy link

edgesite commented Jul 20, 2016

I made a pr #203 to fix this problem. @cygery give it a try.
BTW. don't forget run go install after pull.

@golang golang locked and limited conversation to collaborators Jun 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants