Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-2451

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

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.9.2
    • Component/s: Go - Compiler
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      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.

        Attachments

        1. thrift-2451-v2.patch
          40 kB
          Aleksey Pesternikov

          Issue Links

            Activity

              People

              • Assignee:
                jensg Jens Geyer
                Reporter:
                apesternikov Aleksey Pesternikov
              • Votes:
                0 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: