[
https://issues.apache.org/jira/browse/THRIFT-1833?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13581746#comment-13581746
]
Todd Lipcon commented on THRIFT-1833:
-------------------------------------
Hey Alan. I'm looking over the patch... one quick question:
{code}
- bool is_optional = (*m_iter)->get_req() == t_field::T_OPTIONAL;
+ bool is_optional = (*m_iter)->get_req() != t_field::T_REQUIRED;
{code}
It seems like the difference here is that the new code will include the case of
T_OPT_IN_REQ_OUT, which seems to be "default" requiredness. I assume the reason
you had to do this was to include the union case, but are you sure that this is
correct for the case of a "default" field in a struct? Or should it be {{==
T_OPTIONAL || is_union()}}?
Either way, it seems a little bit inconsistent with the later change:
{code}
- if ((*m_iter)->get_req() != t_field::T_OPTIONAL) {
+ // For union, "default" fields are treated as optional.
+ if (!tstruct->is_union() && (*m_iter)->get_req() != t_field::T_OPTIONAL)
{
{code}
where you've done something more like what I suggested above.
----
{code}
+ for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter) {
+ if ((*f_iter)->get_req() == t_field::T_REQUIRED) {
+ throw "Union must not contain required field";
+ }
{code}
Here, should we explicitly check {{!= t_field::T_OPTIONAL}? Similar to the
above, I'm a little unclear on which 'req' value is expected for union fields.
----
Seems like there is a spurious whitespace change in TestClient.java and also
TestClient.cpp
Otherwise, this seems to make sense.
> Thrift generates incorrect C++ writer for union
> -----------------------------------------------
>
> Key: THRIFT-1833
> URL: https://issues.apache.org/jira/browse/THRIFT-1833
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Compiler
> Affects Versions: 0.9
> Reporter: Alan Choi
> Attachments: Thrift-1833.diff
>
>
> The generated union struct c++ writer would write all union fields. This is
> incorrect. It should only write the field that has been set.
> It causes incompatibility between c++ and Java. A union written by C++ side
> cannot be read by the Java side.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
