New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[C#] fix parse failure for extensions with large field numbers #9591
[C#] fix parse failure for extensions with large field numbers #9591
Conversation
Interesting. Per spec the max extension number is 2^29 - 1, or 536,870,911. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with both the test and the fix - but I'm not sure why the test is being added to the compatibility checks (which are usually about checking that changes don't break anything in previously-generated code).
@@ -415,6 +415,25 @@ public void TestSlowPathAvoidance() | |||
} | |||
} | |||
|
|||
[Test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason you've added this test in here as well? We don't normally do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review.
I didn't know where to add the test cases, so I added them to both files.
It seems unnecessary, so I'll delete this change.
@@ -90,7 +90,7 @@ public static WireType GetTagWireType(uint tag) | |||
/// </summary> | |||
public static int GetTagFieldNumber(uint tag) | |||
{ | |||
return (int) tag >> TagTypeBits; | |||
return (int) (tag >> TagTypeBits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ouch, that was a subtle bug. Thanks for catching it.
Great, thanks. I'll squash and merge on green. |
This shouldn't affect the MacOS Ruby Release, which is the only target still running. Merging. |
Currently, it is failing to parse messages with an extension whose field number is large (> 0x10000000), e.g. https://github.com/google/mediapipe/blob/e6c19885c6d3c6f410c730952aeed2852790d306/mediapipe/calculators/tensor/tensors_to_detections_calculator.proto#L25) because the tag value is cast to
int
before shifting right.