Thrift
  1. Thrift
  2. THRIFT-643

Smalltalk generated code don't load on Squeak3.10.2-7179-basic and PharoCore-1.0-10491rc1 images

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 0.2
    • Fix Version/s: 0.3
    • Component/s: None
    • Labels:
      None
    • Environment:

      Debian GNU/Linux Squeeze
      Smalltalk versions:

      • Squeak3.10.2-7179-basic
      • PharoCore-1.0-10491rc1

      thrift svn revision: 887812

    • Patch Info:
      Patch Available

      Description

      I have found a bug in the generated code for the Smalltalk thrift interface.

      I used thrift svn revision:

      miguel@laptop:~/thrift-svn$ svn info
      Ruta: .
      URL: http://svn.apache.org/repos/asf/incubator/thrift/trunk
      Raíz del repositorio: http://svn.apache.org/repos/asf
      UUID del repositorio: 13f79535-47bb-0310-9956-ffa450edef68
      Revisión: 888521
      Tipo de nodo: directorio
      Agendado: normal
      Autor del último cambio: todd
      Revisión del último cambio: 887812
      Fecha de último cambio: 2009-12-06 18:42:38 -0600 (dom 06 de dic de
      2009)

      I did:

      bootstrap.sh
      configure
      make

      and then I tried to load the thrift smalltalk code from

      lib/st/thrift.st

      in a squeak Squeak3.10.2-7179-basic.zip, pristine image from squeak.org.
      That halted with an error about a duplicated readString method
      declaration. The error is fixed in the attached patch.

      Next I compiled the code for the cassandra thrift:

      https://svn.apache.org/repos/asf/incubator/cassandra/trunk/interface/cassandra.thrift

      with the command:

      ./compiler/cpp/thrift --gen st cassandra.thrift

      and tried to file in the generated code

      gen-st/cassandra.st

      in said image.

      Again the load halted. This time because of the underscores in the
      generated code for the cassandra thrift definition. Squeak (and derived
      smalltalks as Pharo) doesn't allow underscores in variable names (the
      underscore is a legacy character used for variable assigment). Anyway I
      have modified the code for smalltalk generator:

      compiler/cpp/src/generate/t_st_generator.cc

      so that when a variable name in the thrift definition has a "_"
      character, this is removed and the next character is converted to
      uppercase. e.g. a variable like column_name will be generated in
      smalltalk code as columnName. This way the code loads (and works
      correctly) on Squeak.

      The generated code has been tested in the following smalltalk images

      Squeak3.10.2-7179-basic.image
      PharoCore-1.0-10491rc1.image

      and in both of them the new generated code for cassandra works
      correctly.

      I haven't tested the code generation with a thrift spec other than
      cassandra, so maybe this will introduce some bugs. For my case it worked
      great.
      If anyone is using cassandra and want to test it, I installed the
      default cassandra server on localhost port 9160 and tested from Squeak
      and from Pharo by loading

      lib/st/thrift.st
      gen-st/cassandra.st

      Then in a Squeak or Pharo workspace:

      "Insert 10000 values"
      [| cp result client |

      client := CassandraClient binaryOnHost: 'localhost' port: 9160.

      cp := ColumnPath new
      columnFamily: 'Standard1';
      column: 'col1'.

      1 to: 10000 do: [ :i |
      result := client insertKeyspace: 'Keyspace1'
      key: 'row', i asString
      columnPath: cp
      value: 'v', i asString
      timestamp: 1
      consistencyLevel: ((Cassandra enums at: 'ConsistencyLevel') at:
      'QUORUM').]] timeToRun

      And read the values just inserted:

      "Read 10000 values"
      [| cp result client |

      client := CassandraClient binaryOnHost: 'localhost' port: 9160.
      cp := ColumnPath new
      columnFamily: 'Standard1';
      column: 'col1'.

      1 to: 10000 do: [ :i |
      result := client getKeyspace: 'Keyspace1'
      key: 'row', i asString
      columnPath: cp
      consistencyLevel: ((Cassandra enums at: 'ConsistencyLevel') at:
      'QUORUM').]] timeToRun

      Regarding the patch, I am not a C++ expert and the drop_underscores()
      function surely can be improved by someone with more experience with C++
      idioms. Essentially what it does is to convert:

      from -> to
      ----- -----
      a_variable -> aVariable
      a_new_var -> aNewVar
      column_path -> columnPath
      variable -> variable

      Give it a try and if deemed good enough please add it to the thrift
      code.
      Those changes are published under the apache public license, the same as
      the thrift codebase.

      The patch is generated with

      svn diff -x -u > patch.888520

      against revision 888520 of the thrift svn code.

      1. patch.888520
        4 kB
        Miguel Enrique Cobá Martínez
      2. patch.888931
        6 kB
        Miguel Enrique Cobá Martínez

        Activity

        Jake Farrell made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Miguel Enrique Cobá Martínez added a comment -

        Thank you very much

        Show
        Miguel Enrique Cobá Martínez added a comment - Thank you very much
        Todd Lipcon made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Hide
        Miguel Enrique Cobá Martínez added a comment -

        It is more than a month that I sent this patch to the jira. Will someday be merged to the trunk?

        Cheers

        Show
        Miguel Enrique Cobá Martínez added a comment - It is more than a month that I sent this patch to the jira. Will someday be merged to the trunk? Cheers
        Miguel Enrique Cobá Martínez made changes -
        Attachment patch.888931 [ 12427576 ]
        Hide
        Miguel Enrique Cobá Martínez added a comment -

        This new patch implements the comments made by Todd.
        The helper is now in t_generator.h and is named camelcase as that is what it does.
        I found that in t_st_generator.cc there was a sanitize helper that did exactly the same that my function. The problem was that wasn't used everywhere where an underscore could cause problems in Squeak or Pharo. So I refactored the code, moved sanitize to the t_generator.h and changed its name to camelcase. Then changed every reference to sanitize and to my old drop_underscores by calls to camelcase. Other than that, the patch is very similar to the old one.
        Use this instead of the old one.

        Tested on svn revision: 889098 and on the thrift-0.2.0-incubating-rc0.tar.gz code. In both cases the compilation went ok.

        Show
        Miguel Enrique Cobá Martínez added a comment - This new patch implements the comments made by Todd. The helper is now in t_generator.h and is named camelcase as that is what it does. I found that in t_st_generator.cc there was a sanitize helper that did exactly the same that my function. The problem was that wasn't used everywhere where an underscore could cause problems in Squeak or Pharo. So I refactored the code, moved sanitize to the t_generator.h and changed its name to camelcase. Then changed every reference to sanitize and to my old drop_underscores by calls to camelcase. Other than that, the patch is very similar to the old one. Use this instead of the old one. Tested on svn revision: 889098 and on the thrift-0.2.0-incubating-rc0.tar.gz code. In both cases the compilation went ok.
        Hide
        Todd Lipcon added a comment -

        Hi Miguel,

        I think the underscore helper you've added should go in t_generator.h next to the underscore function, since it's essentially the inverse.

        Other than that, patch seems good.

        Show
        Todd Lipcon added a comment - Hi Miguel, I think the underscore helper you've added should go in t_generator.h next to the underscore function, since it's essentially the inverse. Other than that, patch seems good.
        Todd Lipcon made changes -
        Fix Version/s 0.3 [ 12314451 ]
        Fix Version/s 0.2 [ 12313769 ]
        Miguel Enrique Cobá Martínez made changes -
        Field Original Value New Value
        Attachment patch.888520 [ 12427388 ]
        Hide
        Miguel Enrique Cobá Martínez added a comment -

        This patch solves the loading issues with the smalltalk generated code.

        Show
        Miguel Enrique Cobá Martínez added a comment - This patch solves the loading issues with the smalltalk generated code.
        Miguel Enrique Cobá Martínez created issue -

          People

          • Assignee:
            Unassigned
            Reporter:
            Miguel Enrique Cobá Martínez
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development