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
Fix conformance test failures for Google.Protobuf #6910
Fix conformance test failures for Google.Protobuf #6910
Conversation
1e8bef4
to
71ac3e5
Compare
@anandolee @rafi-kamal can you review from protobuf perspective? |
@@ -577,7 +577,32 @@ public uint ReadFixed32() | |||
/// </summary> | |||
public bool ReadBool() | |||
{ | |||
return ReadRawVarint32() != 0; | |||
int result = 0; // can't do AND on bytes, it gets implicitly upcasted to int |
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.
good catch but the fix seems overly complex, just ReadRawVarint64() != 0
should be enough.
FTR that is also what Java does.
return readRawVarint64() != 0; |
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.
Right, I thought about that. Though I also thought that maybe reading an actual varint would be excessive work. The case 99% of the time is that we have at least one byte left in the buffer and the varint is a 1 or 0. ReadRawVarint64 needs at least 10 bytes and does bit shifts and other things just to ultimately check if the whole value has at least one bit set.
@ObsidianMinor I simplified the CodeInputStream changes and also fixed the bool wrapper case. Please doublecheck. |
@@ -845,7 +845,7 @@ public bool MaybeConsumeTag(uint tag) | |||
|
|||
internal static bool? ReadBoolWrapper(CodedInputStream input) | |||
{ | |||
return ReadUInt32Wrapper(input) != 0; | |||
return ReadUInt64Wrapper(input) != 0; |
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.
Unrelated to this PR, I think there might be a bug if ReadUInt64Wrapper returns null - I'll try to check that and add a test in a separate PR.
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.
Currently it never does so I don't think that'll be a problem.
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.
LGTM.
@rafi-kamal can you take a look (and get a permission from @dankurka to merge this fix). |
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.
We did a branch cut for 3.11 yesterday so I don't see any problems unfreezing master and accepting new PRs. @dankurka to confirm.
Fine to merge now? |
Yes, please go ahead. |
It also fixes the exception message for unsupported output formats in Google.Protobuf.Conformance.
cc: @jtattermusch
Fixes #6865