Tree/Recursive struct support in thrift

[djwatson]

Patches to implement tree/list/co-recursive structures in thrift, a la protobufs.

[jfarrell]

Thanks @djwatson created THRIFT-2421 to track it and will review

[snikulov]

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.

[djwatson]

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?

[ben-craig]

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.

[henrique]

Hi Dave,
This destructor is only for structs, isn't it?
The ld error is for *_presult and *_pargs

[snikulov]

@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.

[djwatson]

Thanks guys, fix: #98

[ringerc]

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.

[ringerc]

Created as https://issues.apache.org/jira/browse/THRIFT-4484

[ringerc]

Anyone else facing that issue: workaround is to declare a reference:

struct RecSelf {
   1: i16 item
   2: optional RecSelf &self
 }

Note the &.