Clean up C# code #544
Clean up C# code #544
Conversation
| @@ -408,9 +405,10 @@ void MessageGenerator::GenerateFrameworkMethods(io::Printer* printer) { | |||
| "}\n\n"); | |||
|
|
|||
| // GetHashCode | |||
| // Start with a non-zero value to easily distinguish between null and "empty" messages. | |||
There was a problem hiding this comment.
1 is more used for non-zero value. Better to explain a little why use 17 here.
There was a problem hiding this comment.
I'd be happy to use 1 if that's more common in your experience. I think it makes sense to use a different "empty" hash code for "message", "repeated field" and "map field" though - are 1, 2, 3 fine for you?
(Note that this PR depends on the map PR, so it may be worth reviewing that first.)
There was a problem hiding this comment.
Yes, it is fine. Just add comments to explain it
|
Implementation LGTM. A few comments for the tests |
ProtoDump isn't currently useful, but will be when ToString emits JSON: fixed. ProtoBench: deleted; we should reinstate when there's a common proto3 benchmark. ProtoMunge: delete; not useful enough to merit fixing up. Removed the [TestFixture] from ByteStringTest as Travis uses a recent enough version of NUnit.
- Remove some old proto2-based C#-only messages - Remove the "build" directory which only contained out-of-date files - Remove the csharp_namespace option from proto2 messages - Change "Google.ProtocolBuffers" to "Google.Protobuf" in other messages
|
Okay, this is ready for merge now, if you're happy with it, @anandolee. |
|
yes, LGTM |
|
LGTM (ex-post) |
When --go_out=paths=source_relative:., output filenames are always derived from the input filenames. For example, the output file for a/b/c.proto will always be a/b/c.pb.go, regardless of the presence or absence of a go_package option in the input file. Fixes protocolbuffers#515.
… of returning null), see protocolbuffers#544
Put map entries into unknown fields when unknown enum values are encountered
Put map entries into unknown fields when unknown enum values are encountered
This is relative to pull request #543, which should be merged first.
@jtattermusch @anandolee