[ https://issues.apache.org/jira/browse/THRIFT-2451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13961990#comment-13961990 ]
Ben Sigelman commented on THRIFT-2451: -------------------------------------- A few responses. - It's not clear to me that an optional field with a default value shouldn't be considered "Set"... is that in the spec, or is that your interpretation? (I come from the world of protobufs... in those cases, a field with a default value was essentially always set IIRC). - It's dangerous not to write default values over the wire. Otherwise, if the default value changes, the wire protocol essentially breaks. - Point taken about the GC, though it would be great if you could quantify this with some sort of realistic workload. - Field value accesses were far more broken before IMO -- still error-prone, for sure, but at least not "error-guaranteed" if you depended on the magic "empty" values for various types. FWIW, I can see the argument for adding Get*() accessors... though if we go that route, it might make sense to hide the entire implementation behind a generated interface. (I.e., Set/Get would be the only way to access the underlying structures) My only cautionary note about this is that, if we include a bitfield of some sort to indicate whether fields have been set or not, we basically need to make all of the struct fields un-exported... I wouldn't want to force callers to take responsibility for setting "set" bits along with field values. Obviously this is just my two cents! > 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 > 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. -- This message was sent by Atlassian JIRA (v6.2#6252)