[ 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<RecList> 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/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)