Skip to content

Commit

Permalink
Fix size of oneof fields when they are set to their zero value.
Browse files Browse the repository at this point in the history
We use the proto3 sizers for oneof fields (because they don't have
a pointer in the wrapper struct), but they are always encoded when set,
so we should not skip their zero value.

Fixes #74.
  • Loading branch information
dsymonds committed Sep 10, 2015
1 parent 1dceb1a commit 535a104
Show file tree
Hide file tree
Showing 10 changed files with 122 additions and 72 deletions.
5 changes: 5 additions & 0 deletions jsonpb/jsonpb_test_proto/more_test_objects.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions jsonpb/jsonpb_test_proto/test_objects.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 7 additions & 7 deletions proto/encode.go
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ func size_bool(p *Properties, base structPointer) int {

func size_proto3_bool(p *Properties, base structPointer) int {
v := *structPointer_BoolVal(base, p.field)
if !v {
if !v && !p.oneof {
return 0
}
return len(p.tagcode) + 1 // each bool takes exactly one byte
Expand Down Expand Up @@ -375,7 +375,7 @@ func size_int32(p *Properties, base structPointer) (n int) {
func size_proto3_int32(p *Properties, base structPointer) (n int) {
v := structPointer_Word32Val(base, p.field)
x := int32(word32Val_Get(v)) // permit sign extension to use full 64-bit range
if x == 0 {
if x == 0 && !p.oneof {
return 0
}
n += len(p.tagcode)
Expand Down Expand Up @@ -421,7 +421,7 @@ func size_uint32(p *Properties, base structPointer) (n int) {
func size_proto3_uint32(p *Properties, base structPointer) (n int) {
v := structPointer_Word32Val(base, p.field)
x := word32Val_Get(v)
if x == 0 {
if x == 0 && !p.oneof {
return 0
}
n += len(p.tagcode)
Expand Down Expand Up @@ -466,7 +466,7 @@ func size_int64(p *Properties, base structPointer) (n int) {
func size_proto3_int64(p *Properties, base structPointer) (n int) {
v := structPointer_Word64Val(base, p.field)
x := word64Val_Get(v)
if x == 0 {
if x == 0 && !p.oneof {
return 0
}
n += len(p.tagcode)
Expand Down Expand Up @@ -509,7 +509,7 @@ func size_string(p *Properties, base structPointer) (n int) {

func size_proto3_string(p *Properties, base structPointer) (n int) {
v := *structPointer_StringVal(base, p.field)
if v == "" {
if v == "" && !p.oneof {
return 0
}
n += len(p.tagcode)
Expand Down Expand Up @@ -681,7 +681,7 @@ func (o *Buffer) enc_proto3_slice_byte(p *Properties, base structPointer) error

func size_slice_byte(p *Properties, base structPointer) (n int) {
s := *structPointer_Bytes(base, p.field)
if s == nil {
if s == nil && !p.oneof {
return 0
}
n += len(p.tagcode)
Expand All @@ -691,7 +691,7 @@ func size_slice_byte(p *Properties, base structPointer) (n int) {

func size_proto3_slice_byte(p *Properties, base structPointer) (n int) {
s := *structPointer_Bytes(base, p.field)
if len(s) == 0 {
if len(s) == 0 && !p.oneof {
return 0
}
n += len(p.tagcode)
Expand Down
6 changes: 6 additions & 0 deletions proto/properties.go
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ type Properties struct {
Packed bool // relevant for repeated primitives only
Enum string // set for enum types only
proto3 bool // whether this is known to be a proto3 field; set for []byte only
oneof bool // whether this is a oneof field

Default string // default value
HasDefault bool // whether an explicit default was provided
Expand Down Expand Up @@ -229,6 +230,9 @@ func (p *Properties) String() string {
if p.proto3 {
s += ",proto3"
}
if p.oneof {
s += ",oneof"
}
if len(p.Enum) > 0 {
s += ",enum=" + p.Enum
}
Expand Down Expand Up @@ -305,6 +309,8 @@ func (p *Properties) Parse(s string) {
p.Enum = f[5:]
case f == "proto3":
p.proto3 = true
case f == "oneof":
p.oneof = true
case strings.HasPrefix(f, "def="):
p.HasDefault = true
p.Default = f[4:] // rest of string
Expand Down
1 change: 1 addition & 0 deletions proto/size_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ var SizeTests = []struct {
{"map field with big numeric key", &pb.MessageWithMap{NameMapping: map[int32]string{0xf00d: "om nom nom"}}},

{"oneof not set", &pb.Communique{}},
{"oneof zero int32", &pb.Communique{Union: &pb.Communique_Number{0}}},
{"oneof int32", &pb.Communique{Union: &pb.Communique_Number{3}}},
{"oneof string", &pb.Communique{Union: &pb.Communique_Name{"Rhythmic Fman"}}},
}
Expand Down
74 changes: 37 additions & 37 deletions proto/testdata/test.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 42 additions & 6 deletions protoc-gen-go/generator/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ type getterSymbol struct {
name string
typ string
typeName string // canonical name in proto world; empty for proto.Message and similar
genType bool // whether typ is a generated type (message/group/enum)
genType bool // whether typ contains a generated type (message/group/enum)
}

func (ms *messageSymbol) GenerateAlias(g *Generator, pkg string) {
Expand Down Expand Up @@ -333,15 +333,19 @@ func (ms *messageSymbol) GenerateAlias(g *Generator, pkg string) {
typ := get.typ
val := "(*" + remoteSym + ")(m)." + get.name + "()"
if get.genType {
// typ will be "*pkg.T" (message/group) or "pkg.T" (enum).
// Either of those might have a "[]" prefix if it is repeated.
// Drop the package qualifier since we have hoisted the type into this package.
// typ will be "*pkg.T" (message/group) or "pkg.T" (enum)
// or "map[t]*pkg.T" (map to message/enum).
// The first two of those might have a "[]" prefix if it is repeated.
// Drop any package qualifier since we have hoisted the type into this package.
rep := strings.HasPrefix(typ, "[]")
if rep {
typ = typ[2:]
}
isMap := strings.HasPrefix(typ, "map[")
star := typ[0] == '*'
typ = typ[strings.Index(typ, ".")+1:]
if !isMap { // map types handled lower down
typ = typ[strings.Index(typ, ".")+1:]
}
if star {
typ = "*" + typ
}
Expand Down Expand Up @@ -376,6 +380,30 @@ func (ms *messageSymbol) GenerateAlias(g *Generator, pkg string) {
g.P("}")
continue
}
if isMap {
// Split map[keyTyp]valTyp.
bra, ket := strings.Index(typ, "["), strings.Index(typ, "]")
keyTyp, valTyp := typ[bra+1:ket], typ[ket+1:]
// Drop any package qualifier.
// Only the value type may be foreign.
star := valTyp[0] == '*'
valTyp = valTyp[strings.Index(valTyp, ".")+1:]
if star {
valTyp = "*" + valTyp
}

typ := "map[" + keyTyp + "]" + valTyp
g.P("func (m *", ms.sym, ") ", get.name, "() ", typ, " {")
g.P("o := ", val)
g.P("if o == nil { return nil }")
g.P("s := make(", typ, ", len(o))")
g.P("for k, v := range o {")
g.P("s[k] = (", valTyp, ")(v)")
g.P("}")
g.P("return s")
g.P("}")
continue
}
// Convert imported type into the forwarding type.
val = "(" + typ + ")(" + val + ")"
}
Expand Down Expand Up @@ -863,6 +891,9 @@ func wrapImported(file *descriptor.FileDescriptorProto, g *Generator) (sl []*Imp
for _, index := range file.PublicDependency {
df := g.fileByName(file.Dependency[index])
for _, d := range df.desc {
if d.GetOptions().GetMapEntry() {
continue
}
sl = append(sl, &ImportedDescriptor{common{file}, d})
}
for _, e := range df.enum {
Expand Down Expand Up @@ -1433,13 +1464,18 @@ func (g *Generator) goTag(message *Descriptor, field *descriptor.FieldDescriptor
name += ",proto3"
}
}
return strconv.Quote(fmt.Sprintf("%s,%d,%s%s%s%s%s",
oneof := ""
if field.OneofIndex != nil {
oneof = ",oneof"
}
return strconv.Quote(fmt.Sprintf("%s,%d,%s%s%s%s%s%s",
wiretype,
field.GetNumber(),
optrepreq,
packed,
name,
enum,
oneof,
defaultValue))
}

Expand Down
2 changes: 2 additions & 0 deletions protoc-gen-go/testdata/imp.proto
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ message ImportedMessage {
repeated Owner boss = 6;
repeated ImportedMessage2 memo = 7;

map<string, ImportedMessage2> msg_map = 8;

enum Owner {
DAVE = 1;
MIKE = 2;
Expand Down
Loading

0 comments on commit 535a104

Please sign in to comment.