Thrift
  1. Thrift
  2. THRIFT-1475

Incomplete records generation for Erlang

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.9
    • Component/s: Erlang - Compiler
    • Labels:
    • Environment:

      Gentoo GNU/Linux, Erlang OTP R15B

    • Patch Info:
      Patch Available

      Description

      Thrift compiler for erlang generates incomplete records. It uses erlang string() type for thrift type binary(), and also has not sense to optional values when generates record fields types. Erlang Dialyzer prints warnings on these records so it should be fixed.

        Activity

        Hide
        Hudson added a comment -

        Integrated in Thrift #426 (See https://builds.apache.org/job/Thrift/426/)
        THRIFT-1532/THRIFT-1475 - fix record generation for erlang (Revision 1303663)

        Result = SUCCESS
        molinaro : http://svn.apache.org/viewvc/?view=rev&rev=1303663
        Files :

        • /thrift/trunk/compiler/cpp/src/generate/t_erl_generator.cc
        • /thrift/trunk/lib/erl/Makefile.am
        • /thrift/trunk/lib/erl/test/Thrift1475.thrift
        Show
        Hudson added a comment - Integrated in Thrift #426 (See https://builds.apache.org/job/Thrift/426/ ) THRIFT-1532 / THRIFT-1475 - fix record generation for erlang (Revision 1303663) Result = SUCCESS molinaro : http://svn.apache.org/viewvc/?view=rev&rev=1303663 Files : /thrift/trunk/compiler/cpp/src/generate/t_erl_generator.cc /thrift/trunk/lib/erl/Makefile.am /thrift/trunk/lib/erl/test/Thrift1475.thrift
        Hide
        Anthony Molinaro added a comment -

        I believe the fix for THRIFT-1532 also fixes this. I went a slightly different direction, so see THRIFT-1532 for details. Also please try out the patch and let me know if doesn't work in some cases.

        Show
        Anthony Molinaro added a comment - I believe the fix for THRIFT-1532 also fixes this. I went a slightly different direction, so see THRIFT-1532 for details. Also please try out the patch and let me know if doesn't work in some cases.
        Hide
        Anthony Molinaro added a comment -

        I'm not clear how things are broken, do you have an example .thrift file and an example of how running it with the dialyzer fails?

        I tried

        struct StructA
        {
          1: string a;
          2: binary b;
          3: optional string c;
          4: optional binary d;
          5: required string e;
          6: required binary f;
          7: string g = "foo";
        }
        

        Which generates

        -record(structA, {a = undefined :: string(),
                          b = undefined :: string(),
                          c = undefined :: string(),
                          d = undefined :: string(),
                          e = undefined :: string(),
                          f = undefined :: string(),
                          g = "foo" :: string()}).
        

        Which arguably is not quite correct, but doesn't cause the dialyzer to fail with an error, so I'm not sure what the issue your patch fixes is. Can you include some information on how to reproduce and test this?

        Also, I'm wondering if the type signatures might cause issues in a server. Right not when a server gets a record which fields which are "strings" the fields actually come in as binaries (assumably as an optimization as you might not need them as "strings", so dialyzer might complain about that, not sure).

        Show
        Anthony Molinaro added a comment - I'm not clear how things are broken, do you have an example .thrift file and an example of how running it with the dialyzer fails? I tried struct StructA { 1: string a; 2: binary b; 3: optional string c; 4: optional binary d; 5: required string e; 6: required binary f; 7: string g = "foo" ; } Which generates -record(structA, {a = undefined :: string(), b = undefined :: string(), c = undefined :: string(), d = undefined :: string(), e = undefined :: string(), f = undefined :: string(), g = "foo" :: string()}). Which arguably is not quite correct, but doesn't cause the dialyzer to fail with an error, so I'm not sure what the issue your patch fixes is. Can you include some information on how to reproduce and test this? Also, I'm wondering if the type signatures might cause issues in a server. Right not when a server gets a record which fields which are "strings" the fields actually come in as binaries (assumably as an optimization as you might not need them as "strings", so dialyzer might complain about that, not sure).
        Hide
        Max Treskin added a comment -

        Hey, anybody here?

        Show
        Max Treskin added a comment - Hey, anybody here?
        Hide
        Max Treskin added a comment -

        Patch attached

        Show
        Max Treskin added a comment - Patch attached

          People

          • Assignee:
            Anthony Molinaro
            Reporter:
            Max Treskin
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development