thrift-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Craig Ringer (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (THRIFT-4484) C++ codegen invalid for optional self-membership
Date Wed, 31 Jan 2018 11:26:00 GMT

    [ 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:25 AM:
----------------------------------------------------------------

3On 31 Jan. 2018 16:11, "Jens Geyer (JIRA)" <jira@apache.org> wrote:
{quote}
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*{quote}

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)" <jira@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?


> 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)

Mime
View raw message