[
https://issues.apache.org/jira/browse/THRIFT-2451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13970115#comment-13970115
]
Jens Geyer commented on THRIFT-2451:
------------------------------------
The longer I look at the generated code (either case), the more things I find
that need to be reviewed.
1. As Aleksey has demonstrated, and my tests confirm this, a nil slice and an
empty slice can be distinguished without the pointer mechanics. So that part is
fine with me, and I think we should do it this way.
2. "_I think it's unintuitive that the addition of a default to an optional
field would change the generated Go type of the related field_" - I agree here
as well as on the statement, that a default changes the {{IsSet()}} logic back
to comparing with magic values. But that's exactly what's specified in
THRIFT-2429, so it can't be completely wrong (assumed that THRIFT-2429 defines
what we have/want/should consider as the "truth" here). I'm not completely sure
about this point, but looking at the overall [project
values|http://thrift.apache.org/about] again, the performance gain can't be
ignored, and shouldn't. Bottom line: After thinking about it, I tend slightly
more to the opinion, that we probably should do this as well.
3. I'm a little puzzled about the additional {{GetXxx()}} and {{IsSetXxx()}}
methods that are now generated, but not (or not consequently) used in the
generated code itself. Instead, we have places where a direct {{nil}} check is
performed, sometimes followed by the call to {{IsSetXxx)()}}, which checks the
same pointer against {{nil}} again (BTW, this is also true with the old code,
look at DetailsWanted below).
4. I'm somewhat more puzzled about the fact, that those {{GetXxx()}} and
{{IsSetXxx()}} methods are generated for {{required}} fields as well, always
returning {{true}}. What do we need these for? First, a {{required}} field is
always set, by definition. There is no such thing as an unset {{required}}
field - that's exactly why these methods always return {{true}}.
5. Next, the {{IsSetXxx()}} methods for {{required}} return {{true}}, even
when the (pointer) field is {{nil}}, which it surprisingly can be, because a
pointer-field is generated here as well:
{code}
enum Details {
Everything = 0
StateOnly = 1
StateAndOptions = 2
SomethingElse = 3
}
typedef list< Details> DetailsWanted
struct BaseRequest {
1 : optional string RequestID
}
struct GetMyDetails {
1 : required BaseRequest base_
2 : required string ObjectID
3 : optional DetailsWanted DetailsWanted
}
{code}
gives me
{code}
type GetMyDetails struct {
Base_ *BaseRequest `thrift:"base_,1,required"` // why
pointer?
ObjectID string `thrift:"ObjectID,2,required"`
DetailsWanted DetailsWanted `thrift:"DetailsWanted,3"`
}
func (p *GetMyDetails) GetBase_() *BaseRequest {
return p.Base_ // may be nil?
}
func (p *GetMyDetails) IsSetBase_() bool {
return true // even in the nil case?
}
func (p *GetMyDetails) IsSetDetailsWanted() bool {
return p.DetailsWanted != nil
}
func (p *GetMyDetails) writeField3(oprot thrift.TProtocol) (err error) {
if p.DetailsWanted != nil { // nil test #1
if p.IsSetDetailsWanted() { // nil test #2
...
}
}
}
{code}
I commented all the relevant places in the code above. I'm fully aware that
some points go beyond the initial scope of this ticket, but they belong into
that area as well, and that's why I mention them here.
Comments?
> 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)
