Fix by-value self-inclusion in C++ #1
Conversation
This IDL is fine for languages where everything is by-reference. But for C++, it tries to embed a class by-value into its self, which cannot succeed, and results in compilation failure. See jaegertracing#35 This has been worked around by patching the IDL-generated code; see jaegertracing/jaeger-client-cpp#45 . It turns out we can just tell Thrift to include the optional member by-reference. See the comment in https://issues.apache.org/jira/browse/THRIFT-4484 about `cpp.ref`. Apparently that's now spelled `&` in Thrift IDL. I don't know what effect it has for other languages, the documentation is sparse at best, and doesn't mention this at all. If this doesn't break other languages I think it's the correct fix for the IDL.
| @@ -25,7 +25,7 @@ struct Downstream { | |||
| 3: required string host | |||
| 4: required string port | |||
| 5: required Transport transport | |||
| 6: optional Downstream downstream | |||
| 6: optional Downstream &downstream | |||
There was a problem hiding this comment.
do you have a link to the documentation for this syntax?
There was a problem hiding this comment.
@yurishkuro I don't see it documented anywhere but that doesn't seem to be unusual for Thrift.
It's in test/Recursive.thrift per THRIFT-2421 .
But I think the implementation screwed up support for optional members. It produces bogus code that tries to dereference the __isset member not the member its self, e.g.
/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; }
^~~~~~~~~~~
There was a problem hiding this comment.
Also, the other form, a cpp.ref annotation (https://stackoverflow.com/questions/25631235/what-is-an-annotation-in-apache-thrift-and-what-is-it-used-for) doesn't seem to do anything at all in the C++ code, and only appears in the Go tests.
|
I'm abandoning this due to https://issues.apache.org/jira/browse/THRIFT-4484?focusedCommentId=16349662&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16349662 . Looks like a Thrift bug. Instead I intend to split the Crossdock support out from jaeger cpp-client, since this problem appears to be restricted to the crossdock support related IDL. If not building with crossdock, we can pretend the problem isn't there. |
This IDL is fine for languages where everything is by-reference. But for C++, it tries to embed a class by-value into its self, which cannot succeed, and results in compilation failure. See jaegertracing#35
This has been worked around by patching the IDL-generated code; see jaegertracing/jaeger-client-cpp#45 .
It turns out we can just tell Thrift to include the optional member by-reference. See the comment in https://issues.apache.org/jira/browse/THRIFT-4484 about
cpp.ref. Apparently that's now spelled&in Thrift IDL. I don't know what effect it has for other languages, the documentation is sparse at best, and doesn't mention this at all.If this doesn't break other languages I think it's the correct fix for the IDL.