[ https://issues.apache.org/jira/browse/THRIFT-2451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13973622#comment-13973622 ]
ASF GitHub Bot commented on THRIFT-2451: ---------------------------------------- GitHub user apesternikov opened a pull request: https://github.com/apache/thrift/pull/101 Go inlines THRIFT-2451 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apesternikov/thrift go_inlines Alternatively you can review and apply these changes as the patch at: https://github.com/apache/thrift/pull/101.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #101 ---- commit e6e5dcf3a07cd931183991ff031179b425e2740b Author: Aleksey Pesternikov <a...@alekseys-mbp.att.net> Date: 2014-04-16T14:06:52Z initial change commit f65730e951a4310160a9f7e3e4eeb7e55abd2c55 Author: Aleksey Pesternikov <a...@alekseys-mbp.att.net> Date: 2014-04-16T14:16:03Z no IsSet for required commit 9865f700eb9354d6053994da989a907766c42d1d Author: Aleksey Pesternikov <a...@alekseys-mbp.att.net> Date: 2014-04-17T19:32:13Z inlined required structs commit ca52300c07cefcf553f1ebf35569953c933b2367 Author: Aleksey Pesternikov <a...@alekseys-mbp.att.net> Date: 2014-04-17T19:44:24Z do not use heap for args struct commit 012ca3e512d2bc8822de8a715b4f3d3cae5c0c42 Author: Aleksey Pesternikov <a...@alekseys-mbp.att.net> Date: 2014-04-17T19:52:41Z do not use heap for result struct commit 2fc4afc53ff7db43e08eadeaa30e34bc1fd9c889 Author: Aleksey Pesternikov <a...@alekseys-mbp.att.net> Date: 2014-04-17T21:32:43Z do not set result field on error commit 6e5da0062b139f02dcafe3148cdf02f97c23442a Author: Aleksey Pesternikov <a...@alekseys-mbp.att.net> Date: 2014-04-17T21:57:57Z Jens' thrift source as test case commit 7317957ed708831e280f182f081043fbe9d38a0c Author: Aleksey Pesternikov <ap@alekseys-macbook-pro.local> Date: 2014-04-17T23:43:08Z support for cpp.ref commit 1c4f3efc7b54fd335db633f86faf8c426ae9c87d Author: Aleksey Pesternikov <ap@alekseys-macbook-pro.local> Date: 2014-04-18T00:13:24Z package flag commit c9d7e54f5c5d29c776f42fb861bc9e82da4e542f Author: Aleksey Pesternikov <ap@alekseys-macbook-pro.local> Date: 2014-04-18T00:21:18Z Merge branch 'master' into go_inlines ---- > Do not use pointers for optional fields with defaults. Do not write such > fields if its value set to default. Also, do not use pointers for any > optional fields mapped to go map or slice. generate Get accessors > ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- > > 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 > Attachments: thrift-2451-v2.patch > > > 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)