Update OneofGenerator to support breaking up large switch statements #1866
Conversation
|
Haven't looked at the PR yet, but the comment about the generation option for the size of switches gives be a little pause. I follow the logic on the idea, but I also worry since every option is something we have to continue to support going forward, and to my knowledge, no one has come back with the 500 limit being too large/small yet. @tbkka any thoughts on exposing this as a generation option vs. say just making it a shared constant between the two places? |
|
Would be easy enough to remove the option, but keep it as a constant on GeneratorOptions for sharing. |
|
I too am reluctant to expose unnecessary options. |
There was a problem hiding this comment.
Comparing the OneOf and Enum issues, one thing about the Enum problem is each of the case statements in the switches if a return, i.e. - once we get down to the point that something is matched, the function is exited. But in the current flow, every switch will have to get walked to take the default with the exception of the one that matches. So I wonder if there is anything that can be done about that without getting overly complex. Given it's already serialization code, the small performance hit won't really hurt if we can't, but I figured might as well see if anything could be done.
As far as testing goes, I think for the enum grouping, our "testing" was the reference tests, i.e. - code inspection of how things generate.
I believe the oneof can also get segmented into "chunks" if there is/are any non-oneof fields with a field number that mix in:
message SwitchSplitMessage {
int32 field_06 = 6;
oneof value {
int32 field_02 = 2;
int32 field_01 = 1;
int32 field_05 = 5;
int32 field_03 = 3;
int32 field_04 = 4;
int32 field_12 = 12;
int32 field_08 = 8;
int32 field_09 = 9;
}
int32 field_10 = 10;
}
So you might want to factor that into any test case(s) to things are fully working as intended.
|
I've updated my branch to remove the "MaxCasesInSwitch" option in favor of hardcoding a 500 case limit. I've also updated tests, which now require oneof with 500+ options |
Addresses PR feedback to better test oneof switch splitting: 1. Move fields 2 and 509 outside the oneof as regular fields to verify the split/grouping behavior when regular fields interleave with oneof fields at different positions. 2. Reorder several oneof fields to confirm proper sorting in generated code: - Swap field_010 (now field number 50) with field_050 (now field number 10) - Swap field_100 (now field number 200) with field_200 (now field number 100) - Swap field_300 (now field number 400) with field_400 (now field number 300) The updated code demonstrates: - Switch case statements are sorted by field NUMBER, not field name - Regular fields at positions 2, 251, 502, and 509 properly split the oneof into separate chunks - Each chunk gets its own switch statement in the traverse() method
|
Updated with suggestions |
3 participants
Given a oneof field with ~600 options, the generated traverse function will fail to compile because swift is unable to check the switch is exhaustive in reasonable time. See #1856
This PR follows similar logic to #904 which allows switch statements to be broken up into multiple switch statements (using a
default: breakcase).Instead of hardcoding 500, I've added a new GeneratorOption to support setting the "maxCasesInSwitch" which allows the same number to be used in
EnumGeneratorandOneofGeneratorand allows clients to set this number to whatever suits them based on Swift version or other considerations. This number could also be set to something very high to effectively turn off the feature.The second commit adds tests, I asked claude to help out with this part so please help me in double checking those tests. I was able to run tests successfully locally.