code review 3274043: Encode and decode empty bytes fields correctly.

Encode and decode empty bytes fields correctly.

[mkrautz, 2010-11-28 02:05:03]

Hello r (cc: dsymonds, golang-dev@googlegroups.com), I'd like you to review this change.

[dsymonds, 2010-11-28 03:04:18]

This looks fine to me, modulo my comments. I'll let Rob finish the review. http://codereview.appspot.com/3274043/diff/1/proto/all_test.go File proto/all_test.go (right): http://codereview.appspot.com/3274043/diff/1/proto/all_test.go#newcode781 proto/all_test.go:781: pb.F_BytesRepeated = [][]byte{ []byte{}, } drop the comma http://codereview.appspot.com/3274043/diff/1/proto/all_test.go#newcode789 proto/all_test.go:789: pbd := &GoTest{} use new(GoTest) for consistency (and below) http://codereview.appspot.com/3274043/diff/1/proto/all_test.go#newcode794 proto/all_test.go:794: if !(pbd.F_BytesRequired != nil && len(pbd.F_BytesRequired) == 0) { This reads a bit oddly. Push the ! through the parens. Same below. http://codereview.appspot.com/3274043/diff/1/proto/all_test.go#newcode810 proto/all_test.go:810: pb.F_BytesOptional = nil This is this field's initial value, so setting it here is a no-op. This whole test doesn't do anything beyond what TestEncodeDecode[1-3] already do. http://codereview.appspot.com/3274043/diff/1/proto/all_test.go#newcode845 proto/all_test.go:845: if !(len(pbd.F_BytesRepeated) == 1 && len(pbd.F_BytesRepeated[0]) == 0 && pbd.F_BytesRepeated[0] != nil) { This is also hard to read. Please push the ! throughout the expression. http://codereview.appspot.com/3274043/diff/1/proto/decode.go File proto/decode.go (right): http://codereview.appspot.com/3274043/diff/1/proto/decode.go#newcode482 proto/decode.go:482: if b == nil { If o.DecodeRawBytes returns a nil err then b is always non-nil. You can drop these three lines. http://codereview.appspot.com/3274043/diff/1/proto/encode.go File proto/encode.go (right): http://codereview.appspot.com/3274043/diff/1/proto/encode.go#newcode373 proto/encode.go:373: // check default I don't think this should even be checking the default at all. If the value is the same as the default then it should still be encoded. I think the whole !p.Required block can probably be dropped.

[mkrautz, 2010-11-28 15:00:34]

http://codereview.appspot.com/3274043/diff/1/proto/all_test.go File proto/all_test.go (right): http://codereview.appspot.com/3274043/diff/1/proto/all_test.go#newcode781 proto/all_test.go:781: pb.F_BytesRepeated = [][]byte{ []byte{}, } On 2010/11/28 03:04:19, dsymonds wrote: > drop the comma Done. http://codereview.appspot.com/3274043/diff/1/proto/all_test.go#newcode789 proto/all_test.go:789: pbd := &GoTest{} On 2010/11/28 03:04:19, dsymonds wrote: > use new(GoTest) for consistency > (and below) Done. http://codereview.appspot.com/3274043/diff/1/proto/all_test.go#newcode794 proto/all_test.go:794: if !(pbd.F_BytesRequired != nil && len(pbd.F_BytesRequired) == 0) { On 2010/11/28 03:04:19, dsymonds wrote: > This reads a bit oddly. Push the ! through the parens. Same below. Done. http://codereview.appspot.com/3274043/diff/1/proto/all_test.go#newcode810 proto/all_test.go:810: pb.F_BytesOptional = nil On 2010/11/28 03:04:19, dsymonds wrote: > This is this field's initial value, so setting it here is a no-op. > > This whole test doesn't do anything beyond what TestEncodeDecode[1-3] already > do. Dropped TestEncodeDecodeBytes2. Renamed TestEncodeDecodeBytes3 to TestEncodeDecodeBytes2. http://codereview.appspot.com/3274043/diff/1/proto/all_test.go#newcode845 proto/all_test.go:845: if !(len(pbd.F_BytesRepeated) == 1 && len(pbd.F_BytesRepeated[0]) == 0 && pbd.F_BytesRepeated[0] != nil) { On 2010/11/28 03:04:19, dsymonds wrote: > This is also hard to read. Please push the ! throughout the expression. Done. http://codereview.appspot.com/3274043/diff/1/proto/decode.go File proto/decode.go (right): http://codereview.appspot.com/3274043/diff/1/proto/decode.go#newcode482 proto/decode.go:482: if b == nil { On 2010/11/28 03:04:19, dsymonds wrote: > If o.DecodeRawBytes returns a nil err then b is always non-nil. You can drop > these three lines. Done. http://codereview.appspot.com/3274043/diff/1/proto/encode.go File proto/encode.go (right): http://codereview.appspot.com/3274043/diff/1/proto/encode.go#newcode373 proto/encode.go:373: // check default On 2010/11/28 03:04:19, dsymonds wrote: > I don't think this should even be checking the default at all. If the value is > the same as the default then it should still be encoded. I think the whole > !p.Required block can probably be dropped. I tried to remove it, but the test case failed in proto_test.TestEncodeDecode(1-5). I know nothing of the innards of the protobuf wire format, but it seems like the tests assume that if an optional field has the default value, it shouldn't be encoded in the wire format.

[mkrautz, 2010-11-28 15:01:17]

Hello r, dsymonds (cc: golang-dev@googlegroups.com), Please take another look.

[dsymonds, 2010-11-28 22:49:50]

LGTM Over to Rob. http://codereview.appspot.com/3274043/diff/1/proto/encode.go File proto/encode.go (right): http://codereview.appspot.com/3274043/diff/1/proto/encode.go#newcode373 proto/encode.go:373: // check default On 2010/11/28 15:00:34, mkrautz wrote: > On 2010/11/28 03:04:19, dsymonds wrote: > > I don't think this should even be checking the default at all. If the value is > > the same as the default then it should still be encoded. I think the whole > > !p.Required block can probably be dropped. > > I tried to remove it, but the test case failed in > proto_test.TestEncodeDecode(1-5). > I know nothing of the innards of the protobuf wire format, but it seems like the > tests assume that if an optional field has the default value, it shouldn't be > encoded in the wire format. Okay, that's a bug. Default values should have no impact on encoding/decoding. Just add this: // TODO: remove this block and fix tests and I'll see to it later. http://codereview.appspot.com/3274043/diff/11001/proto/all_test.go File proto/all_test.go (right): http://codereview.appspot.com/3274043/diff/11001/proto/all_test.go#newcode794 proto/all_test.go:794: if pbd.F_BytesRequired == nil { What happened to the len(pbd.F_BytesRequired) check? http://codereview.appspot.com/3274043/diff/11001/proto/all_test.go#newcode800 proto/all_test.go:800: if pbd.F_BytesOptional == nil { len check?

[mkrautz, 2010-11-29 00:09:16]

http://codereview.appspot.com/3274043/diff/1/proto/encode.go File proto/encode.go (right): http://codereview.appspot.com/3274043/diff/1/proto/encode.go#newcode373 proto/encode.go:373: // check default On 2010/11/28 22:49:50, dsymonds wrote: > On 2010/11/28 15:00:34, mkrautz wrote: > > On 2010/11/28 03:04:19, dsymonds wrote: > > > I don't think this should even be checking the default at all. If the value > is > > > the same as the default then it should still be encoded. I think the whole > > > !p.Required block can probably be dropped. > > > > I tried to remove it, but the test case failed in > > proto_test.TestEncodeDecode(1-5). > > I know nothing of the innards of the protobuf wire format, but it seems like > the > > tests assume that if an optional field has the default value, it shouldn't be > > encoded in the wire format. > > Okay, that's a bug. Default values should have no impact on encoding/decoding. > Just add this: > // TODO: remove this block and fix tests > and I'll see to it later. Done. http://codereview.appspot.com/3274043/diff/11001/proto/all_test.go File proto/all_test.go (right): http://codereview.appspot.com/3274043/diff/11001/proto/all_test.go#newcode794 proto/all_test.go:794: if pbd.F_BytesRequired == nil { On 2010/11/28 22:49:50, dsymonds wrote: > What happened to the len(pbd.F_BytesRequired) check? It went away when I simplified the logic in the if block. The thinking was that if the slice is nil, the length will always be 0. However, since we're checking the encoding/decoding, it think it makes sense to explicitly check if the length actually is 0 in cases where the slice != nil. Will re-add. http://codereview.appspot.com/3274043/diff/11001/proto/all_test.go#newcode800 proto/all_test.go:800: if pbd.F_BytesOptional == nil { On 2010/11/28 22:49:50, dsymonds wrote: > len check? Ditto.

[mkrautz, 2010-11-29 00:14:51]

Hello r, dsymonds (cc: golang-dev@googlegroups.com), Please take another look.

[r, 2010-11-29 19:05:08]

I haven't looked at the code yet - just back from vacation - but I have a quick question. What's wrong with decoding an empty byte slice as nil? -rob 

[rsc, 2010-11-29 19:08:48]

On Mon, Nov 29, 2010 at 11:05, Rob 'Commander' Pike <r@golang.org> wrote: > I haven't looked at the code yet - just back from vacation - but I > have a quick question. What's wrong with decoding an empty byte slice > as nil? My guess was that nil means "wasn't present". 

[mkrautz, 2010-11-29 19:16:52]

On 2010/11/29 19:08:48, rsc wrote: > On Mon, Nov 29, 2010 at 11:05, Rob 'Commander' Pike <mailto:r@golang.org> wrote: > > I haven't looked at the code yet - just back from vacation - but I > > have a quick question. What's wrong with decoding an empty byte slice > > as nil? > > My guess was that nil means "wasn't present". That's right. I want to be able to differentiate between a message without a field present (i.e. has_foo() == false in protobuf for C++) and an empty bytes field (has_foo() == true, but length == 0). Context: http://code.google.com/p/goprotobuf/issues/detail?id=6 (Although looking back, the original bug report is also a bit unclear about this.)

[r, 2010-11-29 22:03:19]

http://codereview.appspot.com/3274043/diff/19001/proto/encode.go File proto/encode.go (right): http://codereview.appspot.com/3274043/diff/19001/proto/encode.go#newcode370 proto/encode.go:370: // TODO: remove this block and fix tests how about DOING it?

[dsymonds, 2010-11-29 22:06:41]

FYI http://codereview.appspot.com/3274043/diff/19001/proto/encode.go File proto/encode.go (right): http://codereview.appspot.com/3274043/diff/19001/proto/encode.go#newcode370 proto/encode.go:370: // TODO: remove this block and fix tests On 2010/11/29 22:03:19, r wrote: > how about DOING it? If krautz is up for it, sure, go do it, but he already said he wasn't familiar with the wire encoding format, so I didn't want to burden him with understanding and updating the tests. I can do it as a follow-up.

[r, 2010-11-29 22:08:23]

LGTM

[r, 2010-11-29 22:15:53]

*** Submitted as http://code.google.com/p/goprotobuf/source/detail?r=2790bc36be04 *** Encode and decode empty bytes fields correctly. R=r, dsymonds, rsc CC=golang-dev http://codereview.appspot.com/3274043 Committer: Rob Pike <r@golang.org>