Thrift
  1. Thrift
  2. THRIFT-1518

Generated C++ code only sends the first optional field in the write() function for a struct.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.9
    • Fix Version/s: 0.9
    • Component/s: C++ - Compiler
    • Labels:
      None
    • Patch Info:
      Patch Available

      Description

      The generated C++ code for a struct will only write the first optional field that is set before moving on to writing the required fields, instead it should be checking and writing all of the optional fields if they have been set. Looking at the generated code it is doing "if-else" tests for writing each optional field, this limits it to only writing the first optional field it comes across and should be easily fixed by replacing it with "if" checks instead.

        Issue Links

          Activity

          Hide
          Chris Stylianou added a comment -

          This issue was caused by the changes implemented from this issue.

          Show
          Chris Stylianou added a comment - This issue was caused by the changes implemented from this issue.
          Hide
          Thomas Wiggins added a comment -

          Attached a patch which should resolve the issue.

          Show
          Thomas Wiggins added a comment - Attached a patch which should resolve the issue.
          Hide
          Chris Stylianou added a comment -

          This seems to have resolved my issue thanks!

          Show
          Chris Stylianou added a comment - This seems to have resolved my issue thanks!
          Hide
          Bryan Duxbury added a comment -

          Seems like a pretty big oversight on our part. I would love if you included a test, but I'll understand if that's not going to happen.

          Show
          Bryan Duxbury added a comment - Seems like a pretty big oversight on our part. I would love if you included a test, but I'll understand if that's not going to happen.
          Hide
          Todd Lipcon added a comment -

          Ouch. Do we know what versions this affects? Pretty much cripples the C++ code in those versions.

          Show
          Todd Lipcon added a comment - Ouch. Do we know what versions this affects? Pretty much cripples the C++ code in those versions.
          Hide
          Thomas Wiggins added a comment -

          To Bryan Duxbury: I'm afraid I can't promise anything due to free time, so if any other developer is reading this wants to look at writing a test themselves then go ahead. On the off-chance I do have some time though should it be just the "test/ThriftTest.thrift", "test/cpp/src/TestClient.cpp" and "test/cpp/src/TestServer.cpp" files that need updating with new tests? I've not contributed to Thrift prior to this post so I'm not too familiar with the project layout for writing additional tests.

          Show
          Thomas Wiggins added a comment - To Bryan Duxbury: I'm afraid I can't promise anything due to free time, so if any other developer is reading this wants to look at writing a test themselves then go ahead. On the off-chance I do have some time though should it be just the "test/ThriftTest.thrift", "test/cpp/src/TestClient.cpp" and "test/cpp/src/TestServer.cpp" files that need updating with new tests? I've not contributed to Thrift prior to this post so I'm not too familiar with the project layout for writing additional tests.
          Hide
          Thomas Wiggins added a comment -

          To Todd Lipcon: Happily this doesn't seem to affect any released versions, just the current trunk. The issue which appears to have caused this was committed to SVN on 21/12/2011, 0.8 got released on 29/11/2011.

          Show
          Thomas Wiggins added a comment - To Todd Lipcon: Happily this doesn't seem to affect any released versions, just the current trunk. The issue which appears to have caused this was committed to SVN on 21/12/2011, 0.8 got released on 29/11/2011.
          Hide
          Bryan Duxbury added a comment -

          I just committed Thomas's patch. If there are tests later, we can commit those separately.

          Show
          Bryan Duxbury added a comment - I just committed Thomas's patch. If there are tests later, we can commit those separately.
          Hide
          Todd Lipcon added a comment -

          Gotcha, thanks. I wasn't sure if the 0.8 release had the issue or not, glad to hear it's only trunk.

          Show
          Todd Lipcon added a comment - Gotcha, thanks. I wasn't sure if the 0.8 release had the issue or not, glad to hear it's only trunk.
          Hide
          Hudson added a comment -

          Integrated in Thrift #411 (See https://builds.apache.org/job/Thrift/411/)
          THRIFT-1518. cpp: Generated C++ code only sends the first optional field in the write() function for a struct

          There was some incorrect else if logic added to the CPP generated code, which this patch replaces with the proper functionality.

          Patch: Thomas Wiggins (Revision 1292508)

          Result = FAILURE
          bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1292508
          Files :

          • /thrift/trunk/compiler/cpp/src/generate/t_cpp_generator.cc
          Show
          Hudson added a comment - Integrated in Thrift #411 (See https://builds.apache.org/job/Thrift/411/ ) THRIFT-1518 . cpp: Generated C++ code only sends the first optional field in the write() function for a struct There was some incorrect else if logic added to the CPP generated code, which this patch replaces with the proper functionality. Patch: Thomas Wiggins (Revision 1292508) Result = FAILURE bryanduxbury : http://svn.apache.org/viewvc/?view=rev&rev=1292508 Files : /thrift/trunk/compiler/cpp/src/generate/t_cpp_generator.cc

            People

            • Assignee:
              Thomas Wiggins
              Reporter:
              Chris Stylianou
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development