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] [Updated] (THRIFT-4484) C++ codegen invalid for optional self-membership
Date Wed, 31 Jan 2018 05:00:00 GMT

     [ https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Craig Ringer updated THRIFT-4484:
---------------------------------
    Description: 
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.

  was:
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.

{{{
struct RecSelf {
   1: i16 item
   2: optional RecSelf 
 }
}}}

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:

{{{
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-generation produces

{{{
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 */
};
}}}

Compilation fails with

{{{
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 {
       ^~~~~~~~~~
}}}

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

{{{

class Downstream {
  /* ... */

  std::shared_ptr<Downstream> downstream;

  /* ... */
};

}}}

instead.


> 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