Thrift
  1. Thrift
  2. THRIFT-1366

Delphi generator, lirbrary and unit test.

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.8
    • Component/s: None
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      I wrote Delphi generator, lirbrary and unit test.
      It works with Delphi XE.

      1. THRIFT-1366_delphi_generator_lib_test_v10.zip
        37 kB
        Kenjiro Fukumitsu
      2. THRIFT-1366_delphi_generator_lib_test_v11.zip
        37 kB
        Kenjiro Fukumitsu
      3. THRIFT-1366_delphi_generator_lib_test_v2.zip
        35 kB
        Kenjiro Fukumitsu
      4. THRIFT-1366_delphi_generator_lib_test_v3.zip
        36 kB
        Kenjiro Fukumitsu
      5. THRIFT-1366_delphi_generator_lib_test_v4.zip
        35 kB
        Kenjiro Fukumitsu
      6. THRIFT-1366_delphi_generator_lib_test_v5.zip
        36 kB
        Kenjiro Fukumitsu
      7. THRIFT-1366_delphi_generator_lib_test_v6.zip
        38 kB
        Kenjiro Fukumitsu
      8. THRIFT-1366_delphi_generator_lib_test_v7.zip
        38 kB
        Kenjiro Fukumitsu
      9. THRIFT-1366_delphi_generator_lib_test_v8.zip
        37 kB
        Kenjiro Fukumitsu
      10. THRIFT-1366_delphi_generator_lib_test_v9.zip
        37 kB
        Kenjiro Fukumitsu

        Issue Links

          Activity

          Hide
          Jake Farrell added a comment -

          Kenjiro, can you please attach the patch for this (http://wiki.apache.org/thrift/HowToContribute)

          Show
          Jake Farrell added a comment - Kenjiro, can you please attach the patch for this ( http://wiki.apache.org/thrift/HowToContribute )
          Hide
          Kenjiro Fukumitsu added a comment -

          Hi, Jake.
          Thank you for comment.
          I created a patch and attached it.

          Show
          Kenjiro Fukumitsu added a comment - Hi, Jake. Thank you for comment. I created a patch and attached it.
          Hide
          Jens Geyer added a comment - - edited

          Please have also a look at THRIFT-936

          We are currently working on (another) Delphi CodeGen + library. which is designed to target Delphi 7 up. Compiler will be ready in a few days, library is being currently reviewed. Do you see any chance that we can merge our projects?

          Show
          Jens Geyer added a comment - - edited Please have also a look at THRIFT-936 We are currently working on (another) Delphi CodeGen + library. which is designed to target Delphi 7 up. Compiler will be ready in a few days, library is being currently reviewed. Do you see any chance that we can merge our projects?
          Hide
          Kenjiro Fukumitsu added a comment -

          > Jens.

          It seems difficult to merge our projects, for my generator is based on new language functions, such as generics containers, inner class and so on.
          I think it is the best that they are included into compiler as seperate, if it is possible.
          I respect your effort trying to support Delphi 7 which does not have generics.
          I thought it was too difficult to do.

          Show
          Kenjiro Fukumitsu added a comment - > Jens. It seems difficult to merge our projects, for my generator is based on new language functions, such as generics containers, inner class and so on. I think it is the best that they are included into compiler as seperate, if it is possible. I respect your effort trying to support Delphi 7 which does not have generics. I thought it was too difficult to do.
          Hide
          Jens Geyer added a comment -

          @Bryan,

          what do you think? Would this be an valid option to have two separate generators? The situation here is, that D7 is still very popular, thus I would like to suppport it in order "to spread the word about Thrift".

          @Kenjiro,
          I can act as an reviewer for the XE patch, if you want. Just give me a mail.

          JensG

          Show
          Jens Geyer added a comment - @Bryan, what do you think? Would this be an valid option to have two separate generators? The situation here is, that D7 is still very popular, thus I would like to suppport it in order "to spread the word about Thrift". @Kenjiro, I can act as an reviewer for the XE patch, if you want. Just give me a mail. JensG
          Hide
          Kenjiro Fukumitsu added a comment -

          @Jens
          Thank you. I just send you email.

          Show
          Kenjiro Fukumitsu added a comment - @Jens Thank you. I just send you email.
          Hide
          Jake Farrell added a comment -

          @Jen, we should not have two generators for a given language. If we are to support both it should be handled via flags in the one generator for that given language. There was also a decision awhile ago to try and support the latest version of a given language, though we have used flags for languages when there have been differences between releases (see java and php generators).

          Show
          Jake Farrell added a comment - @Jen, we should not have two generators for a given language. If we are to support both it should be handled via flags in the one generator for that given language. There was also a decision awhile ago to try and support the latest version of a given language, though we have used flags for languages when there have been differences between releases (see java and php generators).
          Hide
          Jens Geyer added a comment - - edited

          The problem is, that we already have two generators now. For the reasons outlined above I still don't like the idea to throw away all the work done to support all these older Delphi versions without generics, that are still used by many, many people. But, OTOH, I also like the new Generics stuff.

          So I think the best thing is to merge the two implementations somehow, and distinguish between them like it is already implemented for python.

          Kenjiro, do you agree?

          Show
          Jens Geyer added a comment - - edited The problem is, that we already have two generators now. For the reasons outlined above I still don't like the idea to throw away all the work done to support all these older Delphi versions without generics, that are still used by many, many people. But, OTOH, I also like the new Generics stuff. So I think the best thing is to merge the two implementations somehow, and distinguish between them like it is already implemented for python. Kenjiro, do you agree?
          Hide
          Kenjiro Fukumitsu added a comment - - edited

          I think python-style command line option is good.
          But I don't think merging our t_XXX_generator.cc is not a good idea,
          because generated code is so different and it seems unnecesary to process for both versions of Delphi in one t_XXX_generator.cc.
          (Plus,since my generator passed unit test and I would like remove the possibilities to include unneccesary degrade.)

          It would be nice if generator factory have the function to understand option(the word after colon in "delphi:XXX").Otherwise we need the wrapper which redirects to actual implementation dependind on options.

          Show
          Kenjiro Fukumitsu added a comment - - edited I think python-style command line option is good. But I don't think merging our t_XXX_generator.cc is not a good idea, because generated code is so different and it seems unnecesary to process for both versions of Delphi in one t_XXX_generator.cc. (Plus,since my generator passed unit test and I would like remove the possibilities to include unneccesary degrade.) It would be nice if generator factory have the function to understand option(the word after colon in "delphi:XXX").Otherwise we need the wrapper which redirects to actual implementation dependind on options.
          Hide
          Kenjiro Fukumitsu added a comment -

          I updated the patch.See THRIFT-1366_delphi_generator_lib_test_v2.zip.

          • Attached the license in front of files.
          • Removed unnecessary code for the prevention of re-entry in ToString() method.
          • Removed the include($I) of private library.
          Show
          Kenjiro Fukumitsu added a comment - I updated the patch.See THRIFT-1366 _delphi_generator_lib_test_v2.zip. Attached the license in front of files. Removed unnecessary code for the prevention of re-entry in ToString() method. Removed the include($I) of private library.
          Hide
          Bryan Duxbury added a comment -

          It sounds like new vs old Delphi are essentially different languages. I wouldn't be against separate generators in that case.

          Show
          Bryan Duxbury added a comment - It sounds like new vs old Delphi are essentially different languages. I wouldn't be against separate generators in that case.
          Hide
          Kenjiro Fukumitsu added a comment -

          > Bryan

          It is a good news.
          It is much easier if it is separated and generated code and library will be more readable.
          They are basically a same language, but recent Delphi has new language features like generics.Using these functions, generated code is nicer, I think.

          Show
          Kenjiro Fukumitsu added a comment - > Bryan It is a good news. It is much easier if it is separated and generated code and library will be more readable. They are basically a same language, but recent Delphi has new language features like generics.Using these functions, generated code is nicer, I think.
          Hide
          Kenjiro Fukumitsu added a comment - - edited

          I updated the patch.

          THRIFT-1366_delphi_generator_lib_test_v3.zip

          • Delphi 2009 and 2010 suport.
          Show
          Kenjiro Fukumitsu added a comment - - edited I updated the patch. THRIFT-1366 _delphi_generator_lib_test_v3.zip Delphi 2009 and 2010 suport.
          Hide
          Jake Farrell added a comment - - edited

          Kenjiro in your patch can you move the test cases from test/delphi to lib/delphi/test (see THRIFT-35)

          Show
          Jake Farrell added a comment - - edited Kenjiro in your patch can you move the test cases from test/delphi to lib/delphi/test (see THRIFT-35 )
          Hide
          Jens Geyer added a comment - - edited

          > sounds like new vs old Delphi are essentially
          > different languages. I wouldn't be against separate
          > generators in that case.

          It depends. As Kenjiro already pointed out, they aren't that different, but some of these differences may produce some headaches. It's not so much about containers, more about language features like nested types/classes and class vars (= static members) that earlier versions do not support.

          However, after thinking about it for a while now I tend to fully agree with Jake. First of all, the new Pascal versions offer much more possibilities and are more up-to-date than older versions. Second, altough I know that Delphi 7 is still widely used (for a number of reasons), it is now 10 years on the market and it will for sure get not more popular within the next years - quite the opposite.

          I think, we should move forward and get the XE version integrated first ASAP. Next step could be to add support for earlier and/or other versions, such as D7 and FPC (Free Pascal).

          > It is much easier if it is separated and generated code
          > and library will be more readable.

          We'll see. One step after another.

          Show
          Jens Geyer added a comment - - edited > sounds like new vs old Delphi are essentially > different languages. I wouldn't be against separate > generators in that case. It depends. As Kenjiro already pointed out, they aren't that different, but some of these differences may produce some headaches. It's not so much about containers, more about language features like nested types/classes and class vars (= static members) that earlier versions do not support. However, after thinking about it for a while now I tend to fully agree with Jake. First of all, the new Pascal versions offer much more possibilities and are more up-to-date than older versions. Second, altough I know that Delphi 7 is still widely used (for a number of reasons), it is now 10 years on the market and it will for sure get not more popular within the next years - quite the opposite. I think, we should move forward and get the XE version integrated first ASAP. Next step could be to add support for earlier and/or other versions, such as D7 and FPC (Free Pascal). > It is much easier if it is separated and generated code > and library will be more readable. We'll see. One step after another.
          Hide
          Kenjiro Fukumitsu added a comment -

          Patch updated.

          • Moved test folder from test/delphi to lib/delphi/test
          • Fixed the problem generating filename with only extension .
          • Fixed Constants class initializer problem.
          • Fixed the problem that generator did not properly include other unit.
          • Moved Constants declaration to the bottom of interface/implemention.
          Show
          Kenjiro Fukumitsu added a comment - Patch updated. Moved test folder from test/delphi to lib/delphi/test Fixed the problem generating filename with only extension . Fixed Constants class initializer problem. Fixed the problem that generator did not properly include other unit. Moved Constants declaration to the bottom of interface/implemention.
          Hide
          Kenjiro Fukumitsu added a comment -

          Patch updated

          • Genarate GUID for interfaces.
          • Added the option to declare binary as ansistring.
          • Added the option to suppress GUID generation.
          • Correct the typos.
          • Added missing line break in test project.
          • Removed test program's wrong parameter.
          • Fixed the problem in Dictionay class when key does not exist.
          • Changed the prefix of interfaces.
          Show
          Kenjiro Fukumitsu added a comment - Patch updated Genarate GUID for interfaces. Added the option to declare binary as ansistring. Added the option to suppress GUID generation. Correct the typos. Added missing line break in test project. Removed test program's wrong parameter. Fixed the problem in Dictionay class when key does not exist. Changed the prefix of interfaces.
          Hide
          Jens Geyer added a comment -

          Thrift test file to check correct treatment of reserved (pascal) keywords and some other special names. Should be included into the TEST folder.

          Show
          Jens Geyer added a comment - Thrift test file to check correct treatment of reserved (pascal) keywords and some other special names. Should be included into the TEST folder.
          Hide
          Kenjiro Fukumitsu added a comment - - edited

          Patch updated.

          • Added GUID for interfaces in library.
          • Fixed the problem that TBinaryProtocol
            throws exception when compiled with "range check" on.
          • Fixed reserved keyword issue.
          • Changed exception fields to hold factory to create exception object.
          Show
          Kenjiro Fukumitsu added a comment - - edited Patch updated. Added GUID for interfaces in library. Fixed the problem that TBinaryProtocol throws exception when compiled with "range check" on. Fixed reserved keyword issue. Changed exception fields to hold factory to create exception object.
          Hide
          Jens Geyer added a comment - - edited

          Revision 6 looks good to me: +1 for this one.

          I checked both code generation and library extensively using Delphi XE. Tried to build a simple independent client project against Cassandra (0.6.0), which worked as expected. Another server/client testcase is included in the package, this also works w/o problems. Problems found during review have been solved.

          I'd just like to see the ReservedKeywords.thrift file to be included as well.

          Good work, Kenjiro!

          Show
          Jens Geyer added a comment - - edited Revision 6 looks good to me: +1 for this one. I checked both code generation and library extensively using Delphi XE. Tried to build a simple independent client project against Cassandra (0.6.0), which worked as expected. Another server/client testcase is included in the package, this also works w/o problems. Problems found during review have been solved. I'd just like to see the ReservedKeywords.thrift file to be included as well. Good work, Kenjiro!
          Hide
          Jens Geyer added a comment -

          Remark @ Committer:
          I just want to mention that the project has some dependencies to boost:

          #include <boost/uuid/uuid.hpp>
          #include <boost/uuid/uuid_generators.hpp>
          #include <boost/uuid/uuid_io.hpp>

          Show
          Jens Geyer added a comment - Remark @ Committer: I just want to mention that the project has some dependencies to boost: #include <boost/uuid/uuid.hpp> #include <boost/uuid/uuid_generators.hpp> #include <boost/uuid/uuid_io.hpp>
          Hide
          Jake Farrell added a comment -

          Thanks Kenjiro and Jens, i'll have some time tonight to go over this and get it committed

          Show
          Jake Farrell added a comment - Thanks Kenjiro and Jens, i'll have some time tonight to go over this and get it committed
          Hide
          Jake Farrell added a comment -

          Jens, can you write a test case that uses the ReservedKeywords idl? dont want to include it if its not getting used by anything

          Show
          Jake Farrell added a comment - Jens, can you write a test case that uses the ReservedKeywords idl? dont want to include it if its not getting used by anything
          Hide
          Jens Geyer added a comment -

          Then leave it out for now.
          I'll provide a separare patch for it soon.

          Show
          Jens Geyer added a comment - Then leave it out for now. I'll provide a separare patch for it soon.
          Hide
          Kenjiro Fukumitsu added a comment - - edited

          I updated the patch. Please see THRIFT-1366_delphi_generator_lib_test_v7.zip.

          I fixed few problems mentioned in the review and the problems I found by myself in the past few days.

          • Changed the code of memory block copy in the library, to make it simpler.
          • Removed the functions in library that are not used anymore.
          • Removed unnecessary comments from generator code.
          • Changed identifier checking fucntion to handle method names of base exception class correctly.
          • Fixed the problem that an indent is inclemented permanently after generating exception class.
          • Changed the generator code to replace white spaces in unitname with underscore.(THRIFT-1383)
          Show
          Kenjiro Fukumitsu added a comment - - edited I updated the patch. Please see THRIFT-1366 _delphi_generator_lib_test_v7.zip. I fixed few problems mentioned in the review and the problems I found by myself in the past few days. Changed the code of memory block copy in the library, to make it simpler. Removed the functions in library that are not used anymore. Removed unnecessary comments from generator code. Changed identifier checking fucntion to handle method names of base exception class correctly. Fixed the problem that an indent is inclemented permanently after generating exception class. Changed the generator code to replace white spaces in unitname with underscore.( THRIFT-1383 )
          Hide
          Jens Geyer added a comment -

          Jake,

          > can you write a test case that uses the ReservedKeywords idl?

          the test case is included in THRIFT-1388.

          Show
          Jens Geyer added a comment - Jake, > can you write a test case that uses the ReservedKeywords idl? the test case is included in THRIFT-1388 .
          Hide
          Jake Farrell added a comment -

          Kenjiro the generator still has a lot of sections of the code commented out as does some of the test cases and parts of the lib. Can you please clean this up, everything else is looking great

          Show
          Jake Farrell added a comment - Kenjiro the generator still has a lot of sections of the code commented out as does some of the test cases and parts of the lib. Can you please clean this up, everything else is looking great
          Hide
          Kenjiro Fukumitsu added a comment - - edited

          @ Jake

          I removed unnecessary comment.
          Please see THRIFT-1366_delphi_generator_lib_test_v8.zip.

          @ Jens.

          I changed generator code a little as it to cast literals to declared type, if "-strict" option is not specified.
          (Now it generates compilable code with DebugProtoTest.thrift.)

          Show
          Kenjiro Fukumitsu added a comment - - edited @ Jake I removed unnecessary comment. Please see THRIFT-1366 _delphi_generator_lib_test_v8.zip. @ Jens. I changed generator code a little as it to cast literals to declared type, if "-strict" option is not specified. (Now it generates compilable code with DebugProtoTest.thrift.)
          Hide
          Jens Geyer added a comment - - edited

          Mmmh. I'm not sure.

          http://wiki.apache.org/thrift/ThriftTypes states that all integers are signed types. Byte is not signed, but ShortInt is.

          I have checked some of the other generators:

          • All signed integers: C++, Java, XSD, AS3, C-GLib
          • Haskell: not 100% sure, but seems to be signed as well
          • C# unsigned "byte", but all others as signed integers
          • Cocoa unsigned "byte", but all others as signed integers

          So (if we leave Delphi out) we have a count of 6:2 for signed 8-bit integers (but remember: i haven't checked them all). Jake, what's the official opinion here? Is a Thrift "byte" signed or unsigned?

          Show
          Jens Geyer added a comment - - edited Mmmh. I'm not sure. http://wiki.apache.org/thrift/ThriftTypes states that all integers are signed types. Byte is not signed, but ShortInt is. I have checked some of the other generators: All signed integers: C++, Java, XSD, AS3, C-GLib Haskell: not 100% sure, but seems to be signed as well C# unsigned "byte", but all others as signed integers Cocoa unsigned "byte", but all others as signed integers So (if we leave Delphi out) we have a count of 6:2 for signed 8-bit integers (but remember: i haven't checked them all). Jake, what's the official opinion here? Is a Thrift "byte" signed or unsigned?
          Hide
          Jake Farrell added a comment -

          byte is an 8-bit signed integer

          Show
          Jake Farrell added a comment - byte is an 8-bit signed integer
          Hide
          Jens Geyer added a comment -

          Rev8 is fine with me , except for the Byte/ShortInt issue of course.

          Show
          Jens Geyer added a comment - Rev8 is fine with me , except for the Byte/ShortInt issue of course.
          Hide
          Kenjiro Fukumitsu added a comment -

          @ Jake, Jens

          Sorry it was my mistake.
          I didn't know byte is defined as signed integer in thrift!!

          I updated the patch.
          Please see THRIFT-1366_delphi_generator_lib_test_v9.zip

          • I changed generator as "byte" is declared as ShortInt.
          • I also changed some library code to fix this issue.
          Show
          Kenjiro Fukumitsu added a comment - @ Jake, Jens Sorry it was my mistake. I didn't know byte is defined as signed integer in thrift!! I updated the patch. Please see THRIFT-1366 _delphi_generator_lib_test_v9.zip I changed generator as "byte" is declared as ShortInt. I also changed some library code to fix this issue.
          Hide
          Kenjiro Fukumitsu added a comment -

          I fixed the exception factory ploblem.
          Please see THRIFT-1366_delphi_generator_lib_test_v10.zip.

          Confirmed with the test case of THRIFT-1391.

          Show
          Kenjiro Fukumitsu added a comment - I fixed the exception factory ploblem. Please see THRIFT-1366 _delphi_generator_lib_test_v10.zip. Confirmed with the test case of THRIFT-1391 .
          Hide
          Kenjiro Fukumitsu added a comment -

          THRIFT-1366_delphi_generator_lib_test_v11.patch

          Changed TestClient.pas to handle exception object properly.

          Show
          Kenjiro Fukumitsu added a comment - THRIFT-1366 _delphi_generator_lib_test_v11.patch Changed TestClient.pas to handle exception object properly.
          Hide
          Jens Geyer added a comment - - edited

          Rev11 tested, works as expected -> +1

          Show
          Jens Geyer added a comment - - edited Rev11 tested, works as expected -> +1
          Hide
          Jake Farrell added a comment -

          Committed, thanks for all the work on this Kenjiro and Jens

          Show
          Jake Farrell added a comment - Committed, thanks for all the work on this Kenjiro and Jens
          Hide
          Hudson added a comment -

          Integrated in Thrift #298 (See https://builds.apache.org/job/Thrift/298/)
          Thrift-1366: Delphi generator, lirbrary and unit test.
          Client: delphi
          Patch: Kenjiro Fukumitsu

          Adding delphi XE generator, lib and unit tests.

          Show
          Hudson added a comment - Integrated in Thrift #298 (See https://builds.apache.org/job/Thrift/298/ ) Thrift-1366: Delphi generator, lirbrary and unit test. Client: delphi Patch: Kenjiro Fukumitsu Adding delphi XE generator, lib and unit tests.
          Hide
          Kenjiro Fukumitsu added a comment -

          @Jake

          Thank you

          But I got email Build in Jenkins server failed,
          because Delphi generator depends on boost 1.42 upper.
          Is it neccesary to add version check to compile these code depends on boost version?

          Show
          Kenjiro Fukumitsu added a comment - @Jake Thank you But I got email Build in Jenkins server failed, because Delphi generator depends on boost 1.42 upper. Is it neccesary to add version check to compile these code depends on boost version?
          Hide
          Jake Farrell added a comment -

          If you depend on that boost version due to the uuid then we will have to check for it in the config step. Go ahead and create a new ticket for the boost version issue and assign it to me and we can discuss adding it to the configure process within that ticket

          Show
          Jake Farrell added a comment - If you depend on that boost version due to the uuid then we will have to check for it in the config step. Go ahead and create a new ticket for the boost version issue and assign it to me and we can discuss adding it to the configure process within that ticket
          Hide
          Kenjiro Fukumitsu added a comment -

          @Jake

          Yes, it is regarding uuid.
          I created new ticket(THRIFT-1396).

          Show
          Kenjiro Fukumitsu added a comment - @Jake Yes, it is regarding uuid. I created new ticket( THRIFT-1396 ).

            People

            • Assignee:
              Kenjiro Fukumitsu
              Reporter:
              Kenjiro Fukumitsu
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development