Thrift
  1. Thrift
  2. THRIFT-127

Erlang assumes that field types are correct and de-synchronizes if they are not

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.1
    • Component/s: Erlang - Library
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      Patch in a moment.

        Issue Links

          Activity

          Show
          David Reiss added a comment - http://gitweb.thrift-rpc.org/?p=thrift.git;a=commitdiff;h=d6c2991
          Hide
          Todd Lipcon added a comment -

          Two comments:

          1) Do other languages support sloppiness between int types? It seems to me that if you change an i32 to an i64 for example, it should allow it (possibly with a warning) given erlang ints don't have sizes. If we're worried about overflow in dependent parts of the system, we could allow the sloppiness only in the upward direction (e.g. receiving an i32 when you expect an i64 isn't a problem).

          2) Line 150: FTypeAtom = thrift_protocol:typeid_to_atom(FType) is now redundant with Type.

          Show
          Todd Lipcon added a comment - Two comments: 1) Do other languages support sloppiness between int types? It seems to me that if you change an i32 to an i64 for example, it should allow it (possibly with a warning) given erlang ints don't have sizes. If we're worried about overflow in dependent parts of the system, we could allow the sloppiness only in the upward direction (e.g. receiving an i32 when you expect an i64 isn't a problem). 2) Line 150: FTypeAtom = thrift_protocol:typeid_to_atom(FType) is now redundant with Type.
          Hide
          David Reiss added a comment -

          1/ No, other languages don't support that sloppiness. Implementing it would require implementing casting rules, which is yucky.
          2/ Made irrelevant by changes.

          The first version didn't actually work. Trying again.
          http://gitweb.thrift-rpc.org/?p=thrift.git;a=commitdiff;h=46a0840

          Show
          David Reiss added a comment - 1/ No, other languages don't support that sloppiness. Implementing it would require implementing casting rules, which is yucky. 2/ Made irrelevant by changes. The first version didn't actually work. Trying again. http://gitweb.thrift-rpc.org/?p=thrift.git;a=commitdiff;h=46a0840
          Hide
          David Reiss added a comment -

          Any chance I can get a second look at this? It is still a fairly small patch (minus indentation changes and pulling out a new function). The current version doesn't allow integer promotion, so I don't think I'm breaking anything.

          Show
          David Reiss added a comment - Any chance I can get a second look at this? It is still a fairly small patch (minus indentation changes and pulling out a new function). The current version doesn't allow integer promotion, so I don't think I'm breaking anything.
          Hide
          Bryan Duxbury added a comment -

          What are we waiting on to commit this?

          Show
          Bryan Duxbury added a comment - What are we waiting on to commit this?
          Hide
          David Reiss added a comment -

          A review from one of the other Erlang devs. Preferably Todd.

          Show
          David Reiss added a comment - A review from one of the other Erlang devs. Preferably Todd.
          Hide
          Eugene Letuchy added a comment -

          The patch is fine. Two minor quibbles:

          1/ I'd rename _Else3 to _Else2 (or just _).
          2/ The message "Skipping fid ~p", just after the diff'd portion, needs to be sync'ed with "Skipping field ~p" and should likely have a reason provided as part of the message.

          Show
          Eugene Letuchy added a comment - The patch is fine. Two minor quibbles: 1/ I'd rename _Else3 to _Else2 (or just _). 2/ The message "Skipping fid ~p", just after the diff'd portion, needs to be sync'ed with "Skipping field ~p" and should likely have a reason provided as part of the message.

            People

            • Assignee:
              David Reiss
              Reporter:
              David Reiss
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development