[jira] [Commented] (THRIFT-2451) Go generated code should not initialize optional fields. Instead, Get accessors should be generated

["Ben Sigelman (JIRA)" <ji...@apache.org>]

    [ 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)