You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by djwatson <gi...@git.apache.org> on 2014/03/21 19:02:49 UTC
[GitHub] thrift pull request: Tree/Recursive struct support in thrift
GitHub user djwatson opened a pull request:
https://github.com/apache/thrift/pull/84
Tree/Recursive struct support in thrift
Patches to implement tree/list/co-recursive structures in thrift, a la protobufs.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/djwatson/thrift trunk
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/84.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #84
----
commit b6134cedf292845e5ed01052919894df6b561bf2
Author: Dave Watson <da...@fb.com>
Date: 2014-03-20T18:12:04Z
Recursive structs support in parser
A common complaint is that you can't express trees or other recursive structures in thrift easily - unlike protobufs. This diff loosens up the parser to allow using structs before they are defined (and uses typedef as a forward declaration).
This diff is actually enough to make recursive types work for some dyamic languages (I tried php, works out of the box!)
Other languages will need forward declarations, or ways to box types, to make this work (i.e. C++ needs both forward decls and a way to express structs as pointers)
commit 477b5fcf31ac71ff8420f96dc16ccf0cf6b35ac5
Author: Dave Watson <da...@fb.com>
Date: 2014-03-21T17:17:39Z
C++ support for recursive structs.
For cpp added a cpp.ref annotation. Users will have to make some recursive types use references to avoid a struct containing itself. Also had to explicitly check for null references when reading/writing, so we know to set the reference to null.
Merge from fb's branch of https://github.com/facebook/fbthrift/commit/9b36d51579bca5ca1c77be333693aa104e8ed141
Fb uses C++11 extensively, this merge changes the type to a raw pointer and adds deletes where necessary. This is *very* similar to how protobufs does it.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Tree/Recursive struct support in thrift
Posted by djwatson <gi...@git.apache.org>.
Github user djwatson commented on the pull request:
https://github.com/apache/thrift/pull/84#issuecomment-39995173
Thanks guys, fix: https://github.com/apache/thrift/pull/98
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Tree/Recursive struct support in thrift
Posted by jfarrell <gi...@git.apache.org>.
Github user jfarrell commented on the pull request:
https://github.com/apache/thrift/pull/84#issuecomment-38307172
Thanks @djwatson created THRIFT-2421 to track it and will review
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #84: Tree/Recursive struct support in thrift
Posted by ringerc <gi...@git.apache.org>.
Github user ringerc commented on the issue:
https://github.com/apache/thrift/pull/84
Anyone else facing that issue: workaround is to declare a reference:
```
struct RecSelf {
1: i16 item
2: optional RecSelf &self
}
```
Note the `&`.
---
[GitHub] thrift pull request: Tree/Recursive struct support in thrift
Posted by henrique <gi...@git.apache.org>.
Github user henrique commented on the pull request:
https://github.com/apache/thrift/pull/84#issuecomment-39985522
Hi Dave,
This destructor is only for structs, isn't it?
The ld error is for *_presult and *_pargs
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #84: Tree/Recursive struct support in thrift
Posted by ringerc <gi...@git.apache.org>.
Github user ringerc commented on the issue:
https://github.com/apache/thrift/pull/84
Created as https://issues.apache.org/jira/browse/THRIFT-4484
---
[GitHub] thrift pull request: Tree/Recursive struct support in thrift
Posted by snikulov <gi...@git.apache.org>.
Github user snikulov commented on the pull request:
https://github.com/apache/thrift/pull/84#issuecomment-39987106
@djwatson Yes. my _types.cpp is linked in.
But as @henrique mentioned _presult and _pargs destructor's are not generated and thus linker gives error.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift issue #84: Tree/Recursive struct support in thrift
Posted by ringerc <gi...@git.apache.org>.
Github user ringerc commented on the issue:
https://github.com/apache/thrift/pull/84
I see a bunch of mutual recursive structure support in the tests, but nothing for an optional single self-referential member, like
```
struct RecSelf {
1: i16 item
2: optional RecSelf
}
```
Thift does not appear to generate sensible code for this. The example I am working with is from the opentracing jaeger toolset, namely the 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
}
```
This works fine in Java, Go, etc. But in C++ it produces the nonsensical 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 */
};
```
in which the class is a by-value member of its self.
I think the ideal solution here would be to adopt something like `std::optional` (c++17 proposal) as a `thrift::optional` and use that instead of the `__isset` stuff for optional members. But backwards compatibility could be a problem.
---
[GitHub] thrift pull request: Tree/Recursive struct support in thrift
Posted by djwatson <gi...@git.apache.org>.
Github user djwatson commented on the pull request:
https://github.com/apache/thrift/pull/84#issuecomment-39975322
The destructor is defined in the .cpp file now:
// Destructor
+ if (tstruct->annotations_.find("final") == tstruct->annotations_.end()) {
+ out <<
+ endl <<
+ indent() << tstruct->get_name() << "::~" << tstruct->get_name() << "() throw() {" << endl;
+ indent_up();
+
+ for (m_iter = members.begin(); m_iter != members.end(); ++m_iter) {
+ if (is_reference(*m_iter)) {
+ out << indent() <<
+ "delete " << (*m_iter)->get_name() << ";" << endl;
+ }
+ }
Is your _types.cpp linked in? Or other repro?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Tree/Recursive struct support in thrift
Posted by jfarrell <gi...@git.apache.org>.
Github user jfarrell closed the pull request at:
https://github.com/apache/thrift/pull/84
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Tree/Recursive struct support in thrift
Posted by snikulov <gi...@git.apache.org>.
Github user snikulov commented on the pull request:
https://github.com/apache/thrift/pull/84#issuecomment-39949481
This change breaks c++ generation.
After this change in compiler/cpp/src/generate/t_cpp_generator.cc
- indent() << "virtual ~" << tstruct->get_name() << "() throw() {}" << endl << endl;
+ indent() << "virtual ~" << tstruct->get_name() << "() throw();" << endl;
linker complains about missed destructor.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: Tree/Recursive struct support in thrift
Posted by ben-craig <gi...@git.apache.org>.
Github user ben-craig commented on the pull request:
https://github.com/apache/thrift/pull/84#issuecomment-39977940
There's a log of a failure in Thrift's nightly build.
https://builds.apache.org/job/Thrift/1123/console
tutorial/shared.thrift and tutorial/tutorial.thrift end up causing problems. The relevant Makefile.am appears to be building in the _types.cpp. I have not looked at the generated source. My current suspicion is that the "final" attribute check is causing problems, but that is currently just a hunch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---