Enum siblings value names #56

merged 2 commits into from Nov 18, 2012


None yet

3 participants


These commits have effect on the names of enum-properties generated by the thrift go generator
The enum values are constants in a go package, therfore, two enums with the same value names would fail. For instance SomeEnumOne.ValueName, SomeEnumTwo.ValueName would generate invalid go code. It would generate 2 constants with the same name (ValueName) in the same go package.
Whereas in java, the value is accessable through SomeEnumOne.ValueName
I've renamed the simple test (generation/interoperability) to have One,Two,Three,Four as value names..
so the enum identifying part is removed (Unefined, Defined). The enum identifying part is now included into the enum value by the go code generator (generates: const DefinedValue_One).

Travis-CI build on this commit fails because of problems at Travis-CI (so it seems).


Looks good enough for me. One nit: Could you document what the problem is with the current implementation in the commit log? I have no doubts that a problem may exist, but I think posterity may require it.

@GeertJohan GeertJohan Renamed enum values in simple.thrift and acompanying tests. The enum …
…identifying part (UndefinedValues, DefinedValues, HeterogenrousValues) is being included in the const name by the thrift go generator as of previous commit.

Removed enumsiblings.thrift, as it's already being tested with simple.thrift now.

I am really glad you caught this stuff. These are some wonderful cleanups.


I may have to look back at where the String() output is used. I think it should be the name of the enum as in the thrift file (with proper capitalization), but not sure.
I understand the reasoning for using the EnumTypeName_EnumName format, but there should be a better way to scope the enums. I don't know what that is right now :/

I'll accept this for now since this is definitely problem with the current implementation, but will need to look at how to do this better in the future.

@pomack pomack merged commit 4e1ad38 into pomack:master Nov 18, 2012

Reply an possible solution in new thread: #57


The FromInt64() is a good idea for usage outside of initialization of the variables. The performance cost is a function call and switch statement versus an initialization of the variable, so extremely minimal.

I was thinking about the third option, but I want the generated output to keep the name as close as possible. Change the var StatusEnum ... to var Status ...

Will need to figure out whether to raise an exception or return a dummy/default value in the FromInt64() function. Probably raise an error.


Yes, simply Status is better yes.
I talked to Matt about this, he also started about the exeption.
Making FromInt64 return (value, error) is a better way I believe..

Do you think its possible to make the EnumType (StatusEnumType) unexported??
That way only the generated code itself can instantiate directly, which is probably just as safe and will make some performance win. I believe only user-code should use the FromIn64 method.

Matt raised the problem with changing IDL. (i.e.: A new enum value unknown to the generated code).
Say the code is to receive an enum value 5, while it only knows about values One(1), Two(2), Three(3), Four(4)... I think that every enum should have a value Unexpected(?). So the received 5 is resolved to Unexpected(?). User code can then eventually switch on the received enum and handle the unexpected case appropriately. I write Unexpected(?) with a question mark because I have no clue what value should be used, as every value can be used in the thrift IDL.
((( Maybe negative value, if that is not possible to set in the .thrift file?, or use pointer and nil this?? )))

As comented on the new issue created for this(#57), I'm creating some example generated code, a more full-featured approach, using the "struct solution".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment