Thrift
  1. Thrift
  2. THRIFT-544

multiple enums with the same key generate invalid code

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.2
    • Fix Version/s: 0.4
    • Component/s: Compiler (General)
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      The current generator produces multiple -define statements with the same name, which isn't valid erlang code (and also isn't valid semantically if we want two different values).

      EnumTest.thrift
      enum MyType1 {
        A = 0,
        B = 1
      }
      
      enum MyType2 {
        A = 2,
        C = 4
      }
      

      produces:

      enumTest_types.hrl
      -ifndef(_enumTest_types_included).
      -define(_enumTest_types_included, yeah).
      
      -define(enumTest_A, 0).
      -define(enumTest_B, 1).
      
      -define(enumTest_A, 2).
      -define(enumTest_C, 4).
      
      -endif.
      

      In the patched version, it produces this:

      enumTest_types.hrl
      -ifndef(_enumTest_types_included).
      -define(_enumTest_types_included, yeah).
      
      -define(enumTest_MyType1_A, 0).
      -define(enumTest_MyType1_B, 1).
      
      -define(enumTest_MyType2_A, 2).
      -define(enumTest_MyType2_C, 4).
      
      -endif.
      

        Activity

        Hide
        David Reiss added a comment -

        This was committed as r982825. It doesn't show up in the "all" tab because it was committed with a tag of "THRIFT-554".

        Show
        David Reiss added a comment - This was committed as r982825. It doesn't show up in the "all" tab because it was committed with a tag of " THRIFT-554 ".
        Hide
        Bryan Duxbury added a comment -

        I made the change that David suggested and committed this.

        Show
        Bryan Duxbury added a comment - I made the change that David suggested and committed this.
        Hide
        David Reiss added a comment -

        I think the patch is correct, but the argument for correctness is very complicated because constants_[name] is a mutating operation that creates an entry if the "name" key is not present. I'd much prefer the condition be "constants_.find(name) != constants.end()"

        Show
        David Reiss added a comment - I think the patch is correct, but the argument for correctness is very complicated because constants_ [name] is a mutating operation that creates an entry if the "name" key is not present. I'd much prefer the condition be "constants_.find(name) != constants.end()"
        Hide
        Bryan Duxbury added a comment -

        I didn't try it out, but its very simple and looks good to me. +1

        Show
        Bryan Duxbury added a comment - I didn't try it out, but its very simple and looks good to me. +1
        Hide
        Ben Taitelbaum added a comment -

        Here's a really simple patch that prevents multiple constants with the same name.

        Show
        Ben Taitelbaum added a comment - Here's a really simple patch that prevents multiple constants with the same name.
        Hide
        Todd Lipcon added a comment -

        I agree with David here - the breaking of backwards compatibility seems like too much to me to be worth it.

        On the other hand, I might support a patch which changes enum generation in erlang to use atoms like 'A' and 'B' instead of -defines at all. It's more erlangy - the bindings use -defines here for historical reasons. Since I'm no longer an active user of the bindings, though, I'll defer to Eugene, Chris, and David (or anyone else who uses them and wants to pipe up) for whether they think that's a good idea.

        Show
        Todd Lipcon added a comment - I agree with David here - the breaking of backwards compatibility seems like too much to me to be worth it. On the other hand, I might support a patch which changes enum generation in erlang to use atoms like 'A' and 'B' instead of -defines at all. It's more erlangy - the bindings use -defines here for historical reasons. Since I'm no longer an active user of the bindings, though, I'll defer to Eugene, Chris, and David (or anyone else who uses them and wants to pipe up) for whether they think that's a good idea.
        Hide
        David Reiss added a comment -

        This is an error in C as well, which is what Thrift's enums are based on. So I think the real solution is to throw an error at code generation time.

        Show
        David Reiss added a comment - This is an error in C as well, which is what Thrift's enums are based on. So I think the real solution is to throw an error at code generation time.
        Hide
        Ben Taitelbaum added a comment -

        This fixes the issue, although it does change the names defined, so it may break existing code, but I can't see any other way around this.

        Show
        Ben Taitelbaum added a comment - This fixes the issue, although it does change the names defined, so it may break existing code, but I can't see any other way around this.

          People

          • Assignee:
            Ben Taitelbaum
            Reporter:
            Ben Taitelbaum
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development