Thrift
  1. Thrift
  2. THRIFT-1318

Incorrect syntax for struct with enum property and default value when value is negative

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.7
    • Fix Version/s: None
    • Component/s: C# - Compiler
    • Labels:
      None
    • Environment:

      Windows 7 / .net 4.0

    • Patch Info:
      Patch Available

      Description

      If I have a struct with a property of type enum with a default value specified, and the default value is negative, then the code generated is incorrect and has a syntax error.

      E.G. thrift file like this:

      enum ReturnType {
          SuccessTypeA = 2
          SuccessUnknownType = 1,
          FailUnknown = -1,
          OtherFailure = -2
      }
      
      struct ReturnInformation {
      	1: required ReturnType RetType = ReturnType.FailUnknown;
      }
      

      Generates a constructor like this:

      
          public MatchInformation() {
            this._MatchType = (MatchTypes)-1;
          }
      
      

      which fails with an exception "To cast a negative value, you must enclose the value in parentheses".

      I have updated the code to always put the enum constant value in parentheses (positive or negative), but need to get the compiler built and tested before I can submit a patch. Any chance someone could do this for me?

        Activity

        Hide
        Thunder Stumpges added a comment -

        Added patch file. Pretty confident this will work! Have NOT gotten the code to build however due to my not having an environment set up to compile the project. Any chance someone could take this for me and run with it??!

        Show
        Thunder Stumpges added a comment - Added patch file. Pretty confident this will work! Have NOT gotten the code to build however due to my not having an environment set up to compile the project. Any chance someone could take this for me and run with it??!
        Hide
        Thunder Stumpges added a comment -

        Got my environment to build the compiler and in testing realized I updated 'render_const_value' but not 'print_const_value' and the latter was in use when making the default constructor. I believe the fix is applicable to 'render_const_value' as well so I left it. The new patch should be correct and works great for me!

        Show
        Thunder Stumpges added a comment - Got my environment to build the compiler and in testing realized I updated 'render_const_value' but not 'print_const_value' and the latter was in use when making the default constructor. I believe the fix is applicable to 'render_const_value' as well so I left it. The new patch should be correct and works great for me!
        Hide
        Thunder Stumpges added a comment -

        Been a long time now, any chance someone could look at this? I have to manually re-apply this patch each time I upgrade versions, and it seems like a simple fix.

        Show
        Thunder Stumpges added a comment - Been a long time now, any chance someone could look at this? I have to manually re-apply this patch each time I upgrade versions, and it seems like a simple fix.
        Hide
        Jens Geyer added a comment -

        Ok, I will, but tomorrow.

        Show
        Jens Geyer added a comment - Ok, I will, but tomorrow.
        Hide
        Jens Geyer added a comment -

        I had some difficulties applying your old SVN patch. I finally came up with this solution which is IMHO much better, as it makes more sense to write the assignments as

        this.property = MyEnumType.SomeValue;
        

        instead of the C-ish casting style like

        this.property = (MyEnumType)(-42);
        

        And since it seems not to be possible to assign values other than the IDL-defined enum values, there is no reason left to use the cast.

        Show
        Jens Geyer added a comment - I had some difficulties applying your old SVN patch. I finally came up with this solution which is IMHO much better, as it makes more sense to write the assignments as this .property = MyEnumType.SomeValue; instead of the C-ish casting style like this .property = (MyEnumType)(-42); And since it seems not to be possible to assign values other than the IDL-defined enum values, there is no reason left to use the cast.
        Hide
        Thunder Stumpges added a comment -

        Thanks, your solution is great. I simply was trying to change as little as possible to keep it working Appreciate the attention to this!

        Show
        Thunder Stumpges added a comment - Thanks, your solution is great. I simply was trying to change as little as possible to keep it working Appreciate the attention to this!
        Hide
        Jens Geyer added a comment -

        Committed.

        Show
        Jens Geyer added a comment - Committed.
        Hide
        Hudson added a comment -

        Integrated in Thrift #629 (See https://builds.apache.org/job/Thrift/629/)
        THRIFT-1318 Incorrect syntax for struct with enum property and default value when value is negative (Revision b3fb3e60925089b6d4c672d7d2cff76a94303b6a)

        Result = FAILURE
        jensg : https://git-wip-us.apache.org/repos/asf?p=thrift.git&a=commit&h=b3fb3e60925089b6d4c672d7d2cff76a94303b6a
        Files :

        • compiler/cpp/src/generate/t_csharp_generator.cc
        Show
        Hudson added a comment - Integrated in Thrift #629 (See https://builds.apache.org/job/Thrift/629/ ) THRIFT-1318 Incorrect syntax for struct with enum property and default value when value is negative (Revision b3fb3e60925089b6d4c672d7d2cff76a94303b6a) Result = FAILURE jensg : https://git-wip-us.apache.org/repos/asf?p=thrift.git&a=commit&h=b3fb3e60925089b6d4c672d7d2cff76a94303b6a Files : compiler/cpp/src/generate/t_csharp_generator.cc

          People

          • Assignee:
            Jens Geyer
            Reporter:
            Thunder Stumpges
          • Votes:
            1 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 1h
              1h
              Remaining:
              Remaining Estimate - 1h
              1h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development