[ https://issues.apache.org/jira/browse/THRIFT-2232?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13803320#comment-13803320 ]
Jens Geyer commented on THRIFT-2232: ------------------------------------ Hi Ben, {quote} The various generated IsSetXYZ() methods just check for magic values of their respective fields. This is obviously broken, and also diverges from the implementation in other languages. {quote} I just ran the test with {code} struct Test { 1: optional string one 2: required string two 3: string three 4: optional i32 four 5: required i32 five 6: i32 six } {code} which gave me this {code} func (p *Test) IsSetOne() bool { return p.One != "" } func (p *Test) IsSetFour() bool { return p.Four != 0 } {code} Whether or not this is sufficient may be questionable (especially IsSetFour() seems a bit heuristic), but I can't find any reference to "reflect" in the Go generator or in IsSetXxx() at all. What am I doing wrong? {quote} Here are some things I'd like to maintain: - do not force users to manually maintain a bit vector of IsSet information {quote} Some languages use setter methods, where the value setter also switches the isset flag to true. Maintaining a bit vector would be an implementation detail, so I agree on that. {quote} The sad thing is that I can't imagine a solution that accomplishes all of the above. Since these are still relatively early days (...), I am inclined to give up the backwards compatibility goal. {quote} The entire Go code has been rewritten not so far ago to keep up with the Go 1.1 release. From that viewpoint, backwards compatibility is relative, but breaking compatibility twice within only a few months sounds not like such a great plan to me. On the other hand, if the implementation participates from it, a few breaking changes that are easy to fix may be an acceptable price. It always depends on the changes, IMHO. {quote} interfaces {quote} One point where I still think interfaces could be a solution is the (still unsolved) problem with complex map keys, THRIFT-2063. {quote} My proposal is to do something akin to what protobufs does to solve this problem. (It's also similar to what other popular packages do for things like this... e.g., the way that the `gorp` ORM package deals with nullable database columns) In particular, we would represent an optional field as a pointer, and use the `nil` value to represent absence. Required fields could either (a) also be represented as pointers and make their containing structs invalid when absent, or (b) be represented as non-pointer fields like the current thrift Go impl does for all fields. {quote} I agree on that. We use similar approaches at other places. {quote} list<>s etc would hopefully be unaffected by this proposal... they can still be empty, and that's all we need as far as presence/absence info (correct me if I'm mistaken!). {quote} Is an empty list really the same as a non-existent list? In my opinion containers should be treaten the same way as structs, at least regarding that point. {quote} The above begs the question of whether there can/should be a central lib of Go thrift helpers that people can add to their projects (rather than simply embedding all of these, redundantly, in every generated thrift Go package). {quote} If you mean that the central helpers library would be something outside of the Thrift source tree, I don't agree. We strive to minimize external dependencies as much as it makes sense in order to reduce problems on the different platforms supported by Thrift, so this would be clearly counter-productive. If we need these helpers, they should be integrated into the Go Thrift runtime package (aka. library). > IsSet* broken in Go > ------------------- > > Key: THRIFT-2232 > URL: https://issues.apache.org/jira/browse/THRIFT-2232 > Project: Thrift > Issue Type: Bug > Components: Go - Compiler > Reporter: Ben Sigelman > Labels: isset > Fix For: 0.9.2 > > > The various generated IsSetXYZ() methods just check for magic values of their > respective fields. This is obviously broken, and also diverges from the > implementation in other languages. > I am willing and able to fix this myself, but I don't want to start on any > impl until we can decide on an approach. > At this point, though, optional fields in Go are basically useless if one's > application makes use of the magic "absence" value. -- This message was sent by Atlassian JIRA (v6.1#6144)