[
https://issues.apache.org/jira/browse/THRIFT-2451?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Jens Geyer updated THRIFT-2451:
-------------------------------
Description:
Currently, for optional fields in struct
{code}
struct pkt {
1: optional string s = "DEFAULT",
2: optional i64 i = 42,
3: optional bool b = false
}
{code}
go compiler generates the following:
{code}
type Pkt struct {
S *string `thrift:"s,1"`
I *int64 `thrift:"i,2"`
B *bool `thrift:"b,3"`
}
func NewPkt() *Pkt {
rval := &Pkt{
S: new(string),
I: new(int64),
B: new(bool),
}
*(rval.S) = "DEFAULT"
*(rval.I) = 42
*(rval.B) = false
return rval
}
func (p *Pkt) IsSetS() bool {
return p.S != nil
}
func (p *Pkt) IsSetI() bool {
return p.I != nil
}
func (p *Pkt) IsSetB() bool {
return p.B != nil
}
{code}
which is wrong in multiple ways:
1. Freshly initialized fields returns IsSetField() true
http://play.golang.org/p/T2pIX80ZJp
This results in
a. wrong semantics: freshly created struct has optional fields set
b. excessive payload produced on serialization (writing field value
instead of skipping it)
2. Additional load on garbage collector
3. accessing field value is complicated and error prone. even without default
value:
{code}
if pkt.IsSetB() && *pkt.B {
//do something for b==true
}
{code}
would work for false default for field b. However, if I change default
value to true, I need to change all occurrences in the code like this:
{code}
if !pkt.IsSetB() || *pkt.B {
//do something for b==true
}
{code}
How to fix that?
there are two ways:
1. get back to generating inlines instead of pointers for optional fields with
default value and compare with "magic value" of default in IsSet*(). could be
tricky since not all types are comparable
http://golang.org/ref/spec#Comparison_operators . notably, slices and maps are
not.
2. approach, used in protobuf: Do not initialize optional fields, generate
Get*() accessors like this:
{code}
var Pkt_B_Default = false
func (p *Pkt) GetB() bool {
if p.B == nil {
return Pkt_B_Default
}
return *p.B
}
{code}
Just to make API uniform, we can also generate accessors for required fields:
{code}
func (p *Pkt) GetB() bool {
return p.B
}
{code}
I'm inclining to implement second approach, but I would like to collect
opinions before I dig into the code.
was:
Currently, for optional fields in struct
struct pkt {
1: optional string s = "DEFAULT",
2: optional i64 i = 42,
3: optional bool b = false
}
go compiler generates the following:
type Pkt struct {
S *string `thrift:"s,1"`
I *int64 `thrift:"i,2"`
B *bool `thrift:"b,3"`
}
func NewPkt() *Pkt {
rval := &Pkt{
S: new(string),
I: new(int64),
B: new(bool),
}
*(rval.S) = "DEFAULT"
*(rval.I) = 42
*(rval.B) = false
return rval
}
func (p *Pkt) IsSetS() bool {
return p.S != nil
}
func (p *Pkt) IsSetI() bool {
return p.I != nil
}
func (p *Pkt) IsSetB() bool {
return p.B != nil
}
which is wrong in multiple ways:
1. Freshly initialized fields returns IsSetField() true
http://play.golang.org/p/T2pIX80ZJp
This results in
a. wrong semantics: freshly created struct has optional fields set
b. excessive payload produced on serialization (writing field value
instead of skipping it)
2. Additional load on garbage collector
3. accessing field value is complicated and error prone. even without default
value:
if pkt.IsSetB() && *pkt.B {
//do something for b==true
}
would work for false default for field b. However, if I change default
value to true, I need to change all occurrences in the code like this:
if !pkt.IsSetB() || *pkt.B {
//do something for b==true
}
How to fix that?
there are two ways:
1. get back to generating inlines instead of pointers for optional fields with
default value and compare with "magic value" of default in IsSet*(). could be
tricky since not all types are comparable
http://golang.org/ref/spec#Comparison_operators . notably, slices and maps are
not.
2. approach, used in protobuf: Do not initialize optional fields, generate
Get*() accessors like this:
var Pkt_B_Default = false
func (p *Pkt) GetB() bool {
if p.B == nil {
return Pkt_B_Default
}
return *p.B
}
Just to make API uniform, we can also generate accessors for required fields:
func (p *Pkt) GetB() bool {
return p.B
}
I'm inclining to implement second approach, but I would like to collect
opinions before I dig into the code.
> Go generated code should not initialize optional fields. Instead, Get
> accessors should be generated
> ---------------------------------------------------------------------------------------------------
>
> Key: THRIFT-2451
> URL: https://issues.apache.org/jira/browse/THRIFT-2451
> Project: Thrift
> Issue Type: Bug
> Components: Go - Compiler
> Reporter: Aleksey Pesternikov
> Assignee: Jens Geyer
>
> Currently, for optional fields in struct
> {code}
> struct pkt {
> 1: optional string s = "DEFAULT",
> 2: optional i64 i = 42,
> 3: optional bool b = false
> }
> {code}
> go compiler generates the following:
> {code}
> type Pkt struct {
> S *string `thrift:"s,1"`
> I *int64 `thrift:"i,2"`
> B *bool `thrift:"b,3"`
> }
> func NewPkt() *Pkt {
> rval := &Pkt{
> S: new(string),
> I: new(int64),
> B: new(bool),
> }
> *(rval.S) = "DEFAULT"
> *(rval.I) = 42
> *(rval.B) = false
> return rval
> }
> func (p *Pkt) IsSetS() bool {
> return p.S != nil
> }
> func (p *Pkt) IsSetI() bool {
> return p.I != nil
> }
> func (p *Pkt) IsSetB() bool {
> return p.B != nil
> }
> {code}
> which is wrong in multiple ways:
> 1. Freshly initialized fields returns IsSetField() true
> http://play.golang.org/p/T2pIX80ZJp
> This results in
> a. wrong semantics: freshly created struct has optional fields set
> b. excessive payload produced on serialization (writing field value
> instead of skipping it)
> 2. Additional load on garbage collector
> 3. accessing field value is complicated and error prone. even without default
> value:
> {code}
> if pkt.IsSetB() && *pkt.B {
> //do something for b==true
> }
> {code}
> would work for false default for field b. However, if I change default
> value to true, I need to change all occurrences in the code like this:
> {code}
> if !pkt.IsSetB() || *pkt.B {
> //do something for b==true
> }
> {code}
> How to fix that?
> there are two ways:
> 1. get back to generating inlines instead of pointers for optional fields
> with default value and compare with "magic value" of default in IsSet*().
> could be tricky since not all types are comparable
> http://golang.org/ref/spec#Comparison_operators . notably, slices and maps
> are not.
> 2. approach, used in protobuf: Do not initialize optional fields, generate
> Get*() accessors like this:
> {code}
> var Pkt_B_Default = false
> func (p *Pkt) GetB() bool {
> if p.B == nil {
> return Pkt_B_Default
> }
> return *p.B
> }
> {code}
> Just to make API uniform, we can also generate accessors for required fields:
> {code}
> func (p *Pkt) GetB() bool {
> return p.B
> }
> {code}
> I'm inclining to implement second approach, but I would like to collect
> opinions before I dig into the code.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
