Uploaded image for project: 'Thrift'
  1. Thrift
  2. THRIFT-4484

C++ codegen invalid for optional self-membership

    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 0.11.0
    • Fix Version/s: None
    • Component/s: C++ - Compiler
    • Labels:
      None
    • 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.

      Description

      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.

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

      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:

      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-generation produces

      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 */
      };
      

      Compilation fails with

      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 {
             ^~~~~~~~~~
      

      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

      
      class Downstream {
        /* ... */
      
        std::shared_ptr<Downstream> downstream;
      
        /* ... */
      };
      
      

      instead.

        Attachments

          Activity

            People

            • Assignee:
              Unassigned
              Reporter:
              ringerc Craig Ringer
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated: