Skip to content
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

Merged

Conversation

@ObsidianMinor
Copy link
Contributor

ObsidianMinor commented Nov 19, 2019

It also fixes the exception message for unsupported output formats in Google.Protobuf.Conformance.

cc: @jtattermusch

Fixes #6865

@googlebot googlebot added the cla: yes label Nov 19, 2019
@ObsidianMinor ObsidianMinor force-pushed the ObsidianMinor:csharp/conformance-failures branch from 1e8bef4 to 71ac3e5 Nov 19, 2019
@jtattermusch

This comment has been minimized.

Copy link
Contributor

jtattermusch commented Nov 20, 2019

@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

This comment has been minimized.

Copy link
@jtattermusch

jtattermusch Nov 20, 2019

Contributor

good catch but the fix seems overly complex, just ReadRawVarint64() != 0 should be enough.

FTR that is also what Java does.

This comment has been minimized.

Copy link
@ObsidianMinor

ObsidianMinor Nov 20, 2019

Author Contributor

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.

@jtattermusch

This comment has been minimized.

Copy link
Contributor

jtattermusch commented Nov 20, 2019

@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;

This comment has been minimized.

Copy link
@jtattermusch

jtattermusch Nov 20, 2019

Contributor

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.

This comment has been minimized.

Copy link
@ObsidianMinor

ObsidianMinor Nov 20, 2019

Author Contributor

Currently it never does so I don't think that'll be a problem.

Copy link
Contributor

jtattermusch left a comment

LGTM.

@jtattermusch

This comment has been minimized.

Copy link
Contributor

jtattermusch commented Nov 20, 2019

@rafi-kamal can you take a look (and get a permission from @dankurka to merge this fix).

Copy link
Contributor

rafi-kamal left a comment

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.

@jtattermusch

This comment has been minimized.

Copy link
Contributor

jtattermusch commented Nov 26, 2019

Fine to merge now?

@rafi-kamal

This comment has been minimized.

Copy link
Contributor

rafi-kamal commented Nov 26, 2019

Yes, please go ahead.

@jtattermusch jtattermusch merged commit 058d5b0 into protocolbuffers:master Nov 26, 2019
56 checks passed
56 checks passed
Bazel Kokoro build successful
Details
Dist artifact installation Kokoro build successful
Details
Linux 32-bit Kokoro build successful
Details
Linux C# Kokoro build successful
Details
Linux C++ Distcheck Kokoro build successful
Details
Linux C++ TC Malloc Kokoro build successful
Details
Linux Golang Kokoro build successful
Details
Linux Java Compatibility Kokoro build successful
Details
Linux Java JDK 7 Kokoro build successful
Details
Linux Java Linkage Monitor Kokoro build successful
Details
Linux Java Oracle 7 Kokoro build successful
Details
Linux JavaScript Kokoro build successful
Details
Linux PHP Kokoro build successful
Details
Linux Python Kokoro build successful
Details
Linux Python 2.7 Kokoro build successful
Details
Linux Python 2.7 C++ Kokoro build successful
Details
Linux Python 3.3 Kokoro build successful
Details
Linux Python 3.3 C++ Kokoro build successful
Details
Linux Python 3.4 Kokoro build successful
Details
Linux Python 3.4 C++ Kokoro build successful
Details
Linux Python 3.5 Kokoro build successful
Details
Linux Python 3.5 C++ Kokoro build successful
Details
Linux Python 3.6 Kokoro build successful
Details
Linux Python 3.6 C++ Kokoro build successful
Details
Linux Python 3.7 Kokoro build successful
Details
Linux Python 3.7 C++ Kokoro build successful
Details
Linux Python C++ Kokoro build successful
Details
Linux Python Compatibility Kokoro build successful
Details
Linux Python Release Kokoro build successful
Details
Linux Ruby 2.3 Kokoro build successful
Details
Linux Ruby 2.4 Kokoro build successful
Details
Linux Ruby 2.5 Kokoro build successful
Details
Linux Ruby 2.6 Kokoro build successful
Details
Linux Ruby Release Kokoro build successful
Details
MacOS C++ Kokoro build successful
Details
MacOS C++ Distcheck Kokoro build successful
Details
MacOS JavaScript Kokoro build successful
Details
MacOS Obj-C CocoaPods Integration Kokoro build successful
Details
MacOS Obj-C OS X Kokoro build successful
Details
MacOS Obj-C iOS Debug Kokoro build successful
Details
MacOS Obj-C iOS Release Kokoro build successful
Details
MacOS PHP5.6 Kokoro build successful
Details
MacOS PHP7.0 Kokoro build successful
Details
MacOS Python Kokoro build successful
Details
MacOS Python C++ Kokoro build successful
Details
MacOS Python Release Kokoro build successful
Details
MacOS Ruby 2.3 Kokoro build successful
Details
MacOS Ruby 2.4 Kokoro build successful
Details
MacOS Ruby 2.5 Kokoro build successful
Details
MacOS Ruby 2.6 Kokoro build successful
Details
MacOS Ruby Release Kokoro build successful
Details
Mergeable Mergeable Run has been Completed!
Details
Windows C# Kokoro build successful
Details
Windows Csharp Release Kokoro build successful
Details
Windows Python Release Kokoro build successful
Details
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.