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