[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16346460#comment-16346460
]
Craig Ringer edited comment on THRIFT-4484 at 1/31/18 11:24 AM:
----------------------------------------------------------------
On 31 Jan. 2018 16:11, "Jens Geyer (JIRA)" <j...@apache.org> wrote:
??
The more interesting question is whether it makes sense to do that.
Let's say you have an instance A that points to itself and then gets
serialized.
Now you de-serialize the bits and get what ... an A that contains another
copy of A = *two instances*
??
I can't say it makes sense to me how it's modelled right now in Jaeger and
- apparently -Zipkin.
But the non-c++ bindings (or at least Go and Java) deal with it, and it's
used in the wild.
So right now it works and is seemingly legal in the IDL but doesn't work in
the C++ generated bindings.
That's why I think it's a bug. Maybe it was a misfeature in the first
place. But shouldn't it either be disallowed in the IDL or supported by all
backends?
was (Author: ringerc):
On 31 Jan. 2018 16:11, "Jens Geyer (JIRA)" <j...@apache.org> wrote:
[ https://issues.apache.org/jira/browse/THRIFT-4484?page=
com.atlassian.jira.plugin.system.issuetabpanels:comment-
tabpanel&focusedCommentId=16346405#comment-16346405 ]
Jens Geyer edited comment on THRIFT-4484 at 1/31/18 8:08 AM:
-------------------------------------------------------------
The more interesting question is whether it makes sense to do that.
Let's say you have an instance A that points to itself and then gets
serialized.
Now you de-serialize the bits and get what ... an A that contains another
copy of A = *two instances*
I can't say it makes sense to me how it's modelled right now in Jaeger and
- apparently -Zipkin.
But the non-c++ bindings (or at least Go and Java) deal with it, and it's
used in the wild.
So right now it works and is seemingly legal in the IDL but doesn't work in
the C++ generated bindings.
That's why I think it's a bug. Maybe it was a misfeature in the first
place. But shouldn't it either be disallowed in the IDL or supported by all
backends?
> C++ codegen invalid for optional self-membership
> ------------------------------------------------
>
> Key: THRIFT-4484
> URL: https://issues.apache.org/jira/browse/THRIFT-4484
> Project: Thrift
> Issue Type: Bug
> Components: C++ - Compiler
> Affects Versions: 0.11.0
> Environment: Thrift 0.10.0 tested, but I don't see a change in
> 0.11.0. Fedora 25. gcc 6.4.1. x86_64.
> Reporter: Craig Ringer
> Priority: Minor
>
> Support was added for self-referential objects in in
> [https://github.com/apache/thrift/pull/84] "Tree/Recursive struct support in
> thrift".
>
> The tests cover objects that are co-recursive, objects that have lists of
> themselves, etc. But there's no test for optional self-containment e.g.
> {code}
> struct RecSelf {
> 1: i16 item
> 2: optional RecSelf
> }
> {code}
> This works fine for languages like Java and Go. But in C++ it generates
> nonsensical code that cannot compile because it contains a by-value member of
> its self and a separate {{isset}} member.
> For example, from opentracing jaeger's IDL:
> {code}
> struct Downstream {
> 1: required string serviceName
> 2: required string serverRole
> 3: required string host
> 4: required string port
> 5: required Transport transport
> 6: optional Downstream downstream
> }
> {code}
> code-generation produces
> {code}
> class Downstream {
> public:
>
> /* blah elided blah */
> virtual ~Downstream() throw();
> std::string serviceName;
> std::string serverRole;
> std::string host;
> std::string port;
> Transport::type transport;
> Downstream downstream;
> _Downstream__isset __isset;
> /* blah elided blah */
> };
> {code}
> Compilation fails with
> {code}
> tracetest_types.h:64:14: error: field ‘downstream’ has incomplete type
> ‘jaegertracing::crossdock::thrift::Downstream’
> Downstream downstream;
> ^~~~~~~~~~
> tracetest_types.h:47:7: note: definition of ‘class
> jaegertracing::crossdock::thrift::Downstream’ is not complete until the
> closing brace
> class Downstream {
> ^~~~~~~~~~
> {code}
> I'd argue that the {{__isset}} model is not ideal, and a
> {{std::expected}}-like "optional" or "maybe" construct would be a lot better.
> But presumably there are historical reasons for that.
> The simplest correct solution would be to generate
> {code}
> class Downstream {
> /* ... */
> std::shared_ptr<Downstream> downstream;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
