Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Won't Fix
    • Affects Version/s: 0.2
    • Fix Version/s: None
    • Component/s: C++ - Compiler
    • Labels:
      None
    • Environment:

      Tested on debian stable (etch)

    • Patch Info:
      Patch Available

      Description

      We're mostly using the C++ interface to thrift, and I wanted to be
      able to initialize structures more easily than a whole series of lines
      that set each of the parameters separately. Attached is a patch that
      adds a constructor to the C++ objects that allow you to fully
      initialize an object in a single call. I added two tests for this,
      one in the DebugProtoTest.cpp file, and one in the
      OptionalRequiredTest.cpp. The patch is against the 20080411p1 release.

      1. structure-constructor.patch
        10 kB
        Eric Anderson
      2. thrift-cpp-constructor-v2.patch
        8 kB
        Alexander Shigin
      3. thrift-cpp-constructor-v3.patch
        9 kB
        Alexander Shigin
      4. thrift-cpp-constructor-v4.patch
        9 kB
        Alexander Shigin
      5. thrift-cpp-constructor-v5.patch
        9 kB
        Alexander Shigin
      6. ASF.LICENSE.NOT.GRANTED--0001-THRIFT-72.-cpp-Add-a-struct-constructor-that-takes.patch
        9 kB
        David Reiss

        Activity

        Hide
        Eric Anderson added a comment -

        Version 2 of the patch; sorts constructor arguments in key order (kinda) negative keys sorted after positive ones, and in descending order.

        Show
        Eric Anderson added a comment - Version 2 of the patch; sorts constructor arguments in key order (kinda) negative keys sorted after positive ones, and in descending order.
        Hide
        David Reiss added a comment -

        In kinda_by_id_less_than, after the first two conditions, you could return abs(a->get_key()) < abs(b->get_key());
        Also, what would you think about just making this a static function of the generator instead of a struct?
        Also, what would you think about naming the arguments lhs and rhs instead?

        Please use spaces instead of tabs for indentation.

        The okay_prefix stuff is unnecessary. You can alias the constructor arguments to the field names and it works fine.

        Show
        David Reiss added a comment - In kinda_by_id_less_than, after the first two conditions, you could return abs(a->get_key()) < abs(b->get_key()); Also, what would you think about just making this a static function of the generator instead of a struct? Also, what would you think about naming the arguments lhs and rhs instead? Please use spaces instead of tabs for indentation. The okay_prefix stuff is unnecessary. You can alias the constructor arguments to the field names and it works fine.
        Hide
        Jérémie BORDIER added a comment -

        I feel like this issue shouldn't be fixed as is, but generalized to other languages that could benefit from this kind of API enhancement in the compiler. What do you guys think ?

        Show
        Jérémie BORDIER added a comment - I feel like this issue shouldn't be fixed as is, but generalized to other languages that could benefit from this kind of API enhancement in the compiler. What do you guys think ?
        Hide
        David Reiss added a comment - - edited

        I have no problem with the basic design, as-is. Some of the other languages already have similar features, and it is tough to require contributors to implement their feature in languages that they do not use.

        Show
        David Reiss added a comment - - edited I have no problem with the basic design, as-is. Some of the other languages already have similar features, and it is tough to require contributors to implement their feature in languages that they do not use.
        Hide
        Jérémie BORDIER added a comment -

        Ok then, +1 !

        Show
        Jérémie BORDIER added a comment - Ok then, +1 !
        Hide
        Alexander Shigin added a comment -

        The first version makes gcc complain about initialization order. The new patch solves the problem.

        Show
        Alexander Shigin added a comment - The first version makes gcc complain about initialization order. The new patch solves the problem.
        Hide
        Alexander Shigin added a comment -

        Add some missing files.

        Show
        Alexander Shigin added a comment - Add some missing files.
        Hide
        Jérémie BORDIER added a comment -

        Patch looks good, +1

        Show
        Jérémie BORDIER added a comment - Patch looks good, +1
        Hide
        Bryan Duxbury added a comment -

        Can a cpp committer weigh in on whether we can commit the latest patch? David?

        Show
        Bryan Duxbury added a comment - Can a cpp committer weigh in on whether we can commit the latest patch? David?
        Hide
        David Reiss added a comment -

        None of my comments were addressed.

        Show
        David Reiss added a comment - None of my comments were addressed.
        Hide
        Alexander Shigin added a comment -

        fix tabs; fix kinda_negative

        Show
        Alexander Shigin added a comment - fix tabs; fix kinda_negative
        Hide
        Alexander Shigin added a comment -

        get rid of ok_prefix

        Show
        Alexander Shigin added a comment - get rid of ok_prefix
        Hide
        David Reiss added a comment -

        Fixed some more tabs. Replaced "\n" with endl. Made the tests pass.

        Show
        David Reiss added a comment - Fixed some more tabs. Replaced "\n" with endl. Made the tests pass.
        Hide
        Alexander Shigin added a comment -

        David, lgtm

        Bryan, I believe assignee should be Erik as the author of initial version of patch, but I can't find him in drop box.

        Show
        Alexander Shigin added a comment - David, lgtm Bryan, I believe assignee should be Erik as the author of initial version of patch, but I can't find him in drop box.
        Hide
        Eric Anderson added a comment -

        I can't commit (no permission), and the related problem is that the patch as written isn't actually useful to us; we want the constructor argument order to match with the order in the thrift file.

        consider the following evolution of a structure:

        struct circle

        { required 1: i32 x; required 2: i32 y; required 3: i32 radius; }

        — V2: (assuming a series of intermediate ones with the necessary optionals)

        struct point

        { required 1: i32 x; required 2: i32 y; }

        struct circle

        { required 4: point center; required 3: i32 radius; }

        The problem is that the latter circle is now initialized as circle(radius, center), which seems inverted to me from the "expected" order for defining such an object. However, I can't reuse the 1 and 2 tags because they are obsolete (as I understand the thrift migration of structure tag rules)

        I'd planned to put in an option to control which way the constructors would go it, but I got busy.

        Out of curiosity, is there a reason that you're replacing \n with endl? They are exactly the same except that endl is equivalent to \n + flush output, so is strictly slower. (http://faq.cprogramming.com/cgi-bin/smartfaq.cgi?id=1043284376&answer=1045690279) – we had this argument internally, and tested it on windows, \n in a string generated \x0a\x0d in a file.

        Show
        Eric Anderson added a comment - I can't commit (no permission), and the related problem is that the patch as written isn't actually useful to us; we want the constructor argument order to match with the order in the thrift file. consider the following evolution of a structure: struct circle { required 1: i32 x; required 2: i32 y; required 3: i32 radius; } — V2: (assuming a series of intermediate ones with the necessary optionals) struct point { required 1: i32 x; required 2: i32 y; } struct circle { required 4: point center; required 3: i32 radius; } The problem is that the latter circle is now initialized as circle(radius, center), which seems inverted to me from the "expected" order for defining such an object. However, I can't reuse the 1 and 2 tags because they are obsolete (as I understand the thrift migration of structure tag rules) I'd planned to put in an option to control which way the constructors would go it, but I got busy. Out of curiosity, is there a reason that you're replacing \n with endl? They are exactly the same except that endl is equivalent to \n + flush output, so is strictly slower. ( http://faq.cprogramming.com/cgi-bin/smartfaq.cgi?id=1043284376&answer=1045690279 ) – we had this argument internally, and tested it on windows, \n in a string generated \x0a\x0d in a file.
        Hide
        David Reiss added a comment -

        The Thrift compiler no longer has any notion of the order that fields are defined in the IDL file. If this poses a problem for you, please see THRIFT-236. I switched to endl because it is the convention in the surrounding code.

        Show
        David Reiss added a comment - The Thrift compiler no longer has any notion of the order that fields are defined in the IDL file. If this poses a problem for you, please see THRIFT-236 . I switched to endl because it is the convention in the surrounding code.
        Hide
        Eric Anderson added a comment -

        I put a comment on THRIFT-236; when we upgrade after the next thrift release we'll probably add an option to support constructor order in IDL order.

        Show
        Eric Anderson added a comment - I put a comment on THRIFT-236 ; when we upgrade after the next thrift release we'll probably add an option to support constructor order in IDL order.
        Hide
        Bryan Duxbury added a comment -

        I see what your issue is here. THRIFT-236 was intended to have an effect on the serialization order, but our solution to this problem inadvertently also had an effect on things like constructors and service method parameter ordering. I tend to agree that this change is undesired and we should probably fix it.

        Show
        Bryan Duxbury added a comment - I see what your issue is here. THRIFT-236 was intended to have an effect on the serialization order, but our solution to this problem inadvertently also had an effect on things like constructors and service method parameter ordering. I tend to agree that this change is undesired and we should probably fix it.
        Hide
        David Reiss added a comment -

        Oh, man. We knew it would affect constructors, but I forgot about service functions. We should definitely fix that.

        Show
        David Reiss added a comment - Oh, man. We knew it would affect constructors, but I forgot about service functions. We should definitely fix that.
        Hide
        Alexander Shigin added a comment -

        The THRIFT-236 is resolved, I believe the David's patch is okay now. Or do I missing something?

        Show
        Alexander Shigin added a comment - The THRIFT-236 is resolved, I believe the David's patch is okay now. Or do I missing something?
        Hide
        David Reiss added a comment -

        Wait. I'm confused. Eric, you say you want the constructor in IDL order, but your original patch used kinda_by_id_less_than. Am I missing something?

        Show
        David Reiss added a comment - Wait. I'm confused. Eric, you say you want the constructor in IDL order, but your original patch used kinda_by_id_less_than. Am I missing something?
        Show
        Alexander Shigin added a comment - The original Eric's patch ( http://publists.facebook.com/pipermail/thrift/2008-July/001071.html ) doesn't do any sorting. It was my mail ( http://publists.facebook.com/pipermail/thrift/2008-July/001072.html ) which proposed field sorting. Mark Slee agreed with me ( http://mail-archives.apache.org/mod_mbox/incubator-thrift-dev/200807.mbox/%3c028001c8e259$c7307eb0$311017ac@IESLEE%3e ).
        Hide
        Roger Meier added a comment -

        issue is too old, please create a new issue and patch if you need this.
        see http://thrift.apache.org/docs/HowToContribute/

        Show
        Roger Meier added a comment - issue is too old, please create a new issue and patch if you need this. see http://thrift.apache.org/docs/HowToContribute/

          People

          • Assignee:
            Unassigned
            Reporter:
            Eric Anderson
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development