[jira] [Comment Edited] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16589667#comment-16589667
]
Craig Ringer edited comment on THRIFT-4484 at 8/23/18 4:01 AM:
---
I think it's related to THRIFT-1528
was (Author: ringerc):
I think it's related to #1528 too
> 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16589667#comment-16589667
]
Craig Ringer commented on THRIFT-4484:
--
I think it's related to #1528 too
> 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Comment Edited] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16349662#comment-16349662
]
Craig Ringer edited comment on THRIFT-4484 at 2/2/18 2:09 AM:
--
If I attempt to actually use {{cpp.ref}} in my code as used in the Go tests it
appears to have no effect, whether I use {{cpp.ref = ""}} or {{cpp.ref =
"true"}}:
{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 (cpp.ref = "true")
}
{code}
it still produces code with a by-value self-inclusion, exactly as if
{{cpp.ref}} were not specified.
If I use {{&}} like the recursive tests do, master, 0.11.0 and 0.9.3 all
produce bogus code, seemingly due to the {{optional}} directive:
{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}
The bogus C++ that tries to reference members of the {{__isset}} not the real
member:
{code}
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.cpp:
In member function ‘uint32_t
jaegertracing::crossdock::thrift::Downstream::read(apache::thrift::protocol::TProtocol*)’:
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.cpp:135:41:
error: ‘jaegertracing::crossdock::thrift::_Downstream__isset {aka struct
jaegertracing::crossdock::thrift::_Downstream__isset}’ has no member named
‘serviceName’
if (this->downstream->__isset.serviceName) { wasSet = true; }
^~~
{code}
so it seems to me that {{&}} is broken with {{optional}}.
A trivial test addition doesn't seem to show this
{code}
diff --git a/test/Recursive.thrift b/test/Recursive.thrift
index c982582..160c2c7 100644
--- a/test/Recursive.thrift
+++ b/test/Recursive.thrift
@@ -39,9 +39,15 @@ struct VectorTest {
1: list lister;
}
+struct SelfRec {
+ 1: i16 myvalue
+ 2: optional SelfRec &secondme
+}
+
service TestService
{
RecTree echoTree(1:RecTree tree)
RecList echoList(1:RecList lst)
CoRec echoCoRec(1:CoRec item)
+ SelfRec echoSelfRec(1:SelfRec item)
}
{code}
because nothing dereferences {{secondme}} to get {{myvalue}}. I don't know
enough Thrift to know how to do that, I'm working on an issue in someone
else's IDL.
was (Author: ringerc):
If I attempt to actually use {{cpp.ref}} in my code as used in the Go tests it
appears to have no effect, whether I use {{cpp.ref = ""}} or {{cpp.ref =
"true"}}:
{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 (cpp.ref = "true")
}
{code}
it still produces code with a by-value self-inclusion, exactly as if
{{cpp.ref}} were not specified.
If I use {{&}} like the recursive tests do, master, 0.11.0 and 0.9.3 all
produce bogus code, seemingly due to the {{optional}} directive:
{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}
The bogus C++ that tries to reference members of the {{__isset}} not the real
member:
{code}
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.cpp:
In member function ‘uint32_t
jaegertracing::crossdock::thrift::Downstream::read(apache::thrift::protocol::TProtocol*)’:
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.cpp:135:41:
error: ‘jaegertracing::crossdock::thrift::_Downstream__isset {aka struct
jaegertracing::crossdock::thrift::_Downstream__isset}’ has no member named
‘serviceName’
if (this->downstream->__isset.serviceName) { wasSet = true; }
^~~
{code}
so it seems to me that {{&}} is broken with {{optional}}.
> 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/thrif
[jira] [Comment Edited] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16349631#comment-16349631
]
Craig Ringer edited comment on THRIFT-4484 at 2/2/18 2:05 AM:
--
Also, the regression tests show that both {{cpp.ref}} and {{&}} are
used/supported. {{cpp.ref}} appears in
{{lib/go/test/RefAnnotationFieldsTest.thrift}}, and {{&}} appears in
{{test/Recursive.thrift}}. Neither appear to be documented. I'm not sure what's
going on.
EDIT: Per
https://issues.apache.org/jira/browse/THRIFT-2471?focusedCommentId=16349640&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16349640
this looks like an oversight when merging, the {{cpp.ref}} annotation appears
to do nothing.
BTW, {{&}} seems to be undocumented.
was (Author: ringerc):
Also, the regression tests show that both {{cpp.ref}} and {{&}} are
used/supported. {{cpp.ref}} appears in
{{lib/go/test/RefAnnotationFieldsTest.thrift}}, and {{&}} appears in
{{test/Recursive.thrift}}. Neither appear to be documented. I'm not sure what's
going on.
> 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16349662#comment-16349662
]
Craig Ringer commented on THRIFT-4484:
--
If I attempt to actually use {{cpp.ref}} in my code as used in the Go tests it
appears to have no effect, whether I use {{cpp.ref = ""}} or {{cpp.ref =
"true"}}:
{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 (cpp.ref = "true")
}
{code}
it still produces code with a by-value self-inclusion, exactly as if
{{cpp.ref}} were not specified.
If I use {{&}} like the recursive tests do, master, 0.11.0 and 0.9.3 all
produce bogus code, seemingly due to the {{optional}} directive:
{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}
The bogus C++ that tries to reference members of the {{__isset}} not the real
member:
{code}
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.cpp:
In member function ‘uint32_t
jaegertracing::crossdock::thrift::Downstream::read(apache::thrift::protocol::TProtocol*)’:
/home/craig/projects/2Q/opentracing-jaeger-cpp-client/src/jaegertracing/thrift-gen/tracetest_types.cpp:135:41:
error: ‘jaegertracing::crossdock::thrift::_Downstream__isset {aka struct
jaegertracing::crossdock::thrift::_Downstream__isset}’ has no member named
‘serviceName’
if (this->downstream->__isset.serviceName) { wasSet = true; }
^~~
{code}
so it seems to me that {{&}} is broken with {{optional}}.
> 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-2471) Make cpp.ref annotation language agnostic
[
https://issues.apache.org/jira/browse/THRIFT-2471?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16349640#comment-16349640
]
Craig Ringer commented on THRIFT-2471:
--
I think a mistake may have been made in this commit, because {{cpp.ref}} still
exists in the tree but only apparently for the Go compiler:
{code}
CHANGES:* [THRIFT-2471] - Make cpp.ref annotation language agnostic
compiler/cpp/src/thrift/generate/t_go_generator.cc: if
(tfield->annotations_.count("cpp.ref") != 0) {
lib/go/test/RefAnnotationFieldsTest.thrift: 1: optional string s = "DEFAULT"
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 2: optional i64 i = 42 (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 3: optional bool b = false (cpp.ref
= ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 4: optional string s2 (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 5: optional i64 i2 (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 6: optional bool b2 (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 7: optional structA aa (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 9: optional list l (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 10: optional list l2 = [1, 2]
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 11: optional map m
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 12: optional map m2 =
{1:2, 3:4} (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 13: optional binary bin (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 14: optional binary bin2 = "asdf"
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 15: required string ref_s =
"DEFAULT" (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 16: required i64 ref_i = 42
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 17: required bool ref_b = false
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 18: required string ref_s2 (cpp.ref
= ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 19: required i64 ref_i2 (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 20: required bool ref_b2 (cpp.ref =
""),
lib/go/test/RefAnnotationFieldsTest.thrift: 21: required structA ref_aa
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 22: required list ref_l
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 23: required list ref_l2 = [1,
2] (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 24: required map ref_m
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 25: required map ref_m2 =
{1:2, 3:4} (cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 26: required binary ref_bin
(cpp.ref = ""),
lib/go/test/RefAnnotationFieldsTest.thrift: 27: required binary ref_bin2 =
"asdf" (cpp.ref = ""),
{code}
and a {{git grep '\'}} doesn't reveal anything that looks relevant; in
particular nothing at all in the c++ codegen.
Looks to me like support in Go was missed when converting this to the {{&}}
form.
There's also another oversight where it produces bad code for {{optional}}
members, but I'll raise a new bug for that.
> Make cpp.ref annotation language agnostic
> -
>
> Key: THRIFT-2471
> URL: https://issues.apache.org/jira/browse/THRIFT-2471
> Project: Thrift
> Issue Type: Improvement
> Components: Compiler (General)
>Reporter: Jens Geyer
>Assignee: Jens Geyer
>Priority: Major
> Fix For: 0.9.2
>
> Attachments: boost_1.40.0__to__1.54.0-patch
>
>
> The proposal is to make the new {{cpp.ref}} annotation introduced with
> THRIFT-2421 language agnostic instead of a C++ specialty only.
> The annotation changes inlined nested structs into pointers to structs. This
> behaviour is potentially relevant with all languages using value semantics
> for nested structs etc.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Comment Edited] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16349631#comment-16349631
]
Craig Ringer edited comment on THRIFT-4484 at 2/2/18 1:43 AM:
--
Also, the regression tests show that both {{cpp.ref}} and {{&}} are
used/supported. {{cpp.ref}} appears in
{{lib/go/test/RefAnnotationFieldsTest.thrift}}, and {{&}} appears in
{{test/Recursive.thrift}}. Neither appear to be documented. I'm not sure what's
going on.
was (Author: ringerc):
Also, the regression tests show that both {{cpp.ref}} and {{&}} are
used/supported. {{cpp.ref}} appears in
{{lib/go/test/RefAnnotationFieldsTest.thrift}}, and {{&}} appears in
{{test/Recursive.thrift}}. Neither appear to be documented. I'm not sure what's
going on.
> 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16349631#comment-16349631
]
Craig Ringer commented on THRIFT-4484:
--
Also, the regression tests show that both {{cpp.ref}} and {{&}} are
used/supported. {{cpp.ref}} appears in
{{lib/go/test/RefAnnotationFieldsTest.thrift}}, and {{&}} appears in
{{test/Recursive.thrift}}. Neither appear to be documented. I'm not sure what's
going on.
> 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Comment Edited] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16349623#comment-16349623
]
Craig Ringer edited comment on THRIFT-4484 at 2/2/18 1:18 AM:
--
[~davejwatson] Hrm, too soon. {{&}} generates bogus code that tries to
reference properties of {{__isset}} members as if they're the main member. I
can't find it documented or find it in the tests either. I thought from reading
the discussion on {{cpp.ref}} in THRIFT-2471 that it was renamed to {{&}}
before commit, but maybe not. No idea what's going on now.
was (Author: ringerc):
[~davejwatson] Hrm, too soon. {{&}} generates bogus code that tries to
reference properties of {{__isset}} members as if they're the main member. I
can't find it documented or find it in the tests either. I thought from reading
the discussion on {{cpp.ref}} that it was renamed to {{&}} before commit, but
maybe not. No idea what's going on now.
> 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16349623#comment-16349623
]
Craig Ringer commented on THRIFT-4484:
--
[~davejwatson] Hrm, too soon. {{&}} generates bogus code that tries to
reference properties of {{__isset}} members as if they're the main member. I
can't find it documented or find it in the tests either. I thought from reading
the discussion on {{cpp.ref}} that it was renamed to {{&}} before commit, but
maybe not. No idea what's going on now.
> 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16348660#comment-16348660
]
Craig Ringer commented on THRIFT-4484:
--
[~davejwatson] Brilliant! Apparently it's now spelled "&", e.g.
{code}
-6: optional Downstream downstream
+6: optional Downstream &downstream
{code}
but that does the job.
It'd be nice if Thrift detected a struct that self-referenced by value, bailed
out with a compile error and offered this as a hint. But hopefully now others
will find this bug if they search for the error messages, and as I've spent
less than a day looking at thrift IDL (working on this Jaeger issue) let alone
the backend, it's sure beyond me.
Thanks so much for the pointer. Heh.
> 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Comment Edited] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
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)" 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)" 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Comment Edited] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
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)" 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)" 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
https://issues.apache.org/jira/browse/THRIFT-4484?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16346460#comment-16346460
]
Craig Ringer commented on THRIFT-4484:
--
On 31 Jan. 2018 16:11, "Jens Geyer (JIRA)" 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;
> /* ... */
> };
> {code}
> instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Updated] (THRIFT-4484) C++ codegen invalid for optional self-membership
[
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;
/* ... */
};
{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;
/* ... */
};
}}}
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 genera
[jira] [Created] (THRIFT-4484) C++ codegen invalid for optional self-membership
Craig Ringer created THRIFT-4484:
Summary: 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
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;
/* ... */
};
}}}
instead.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
