Issue619 #621
Issue619 #621
Conversation
…en a proto.Buffer and proto.Marshal with different proto.Buffer sizes.
…. This allows us to marshal at the correct place when the buffer contains a larger backing buffer. (gogo#619)
| @@ -189,8 +189,22 @@ type Marshaler interface { | |||
| // prefixed by a varint-encoded length. | |||
| func (p *Buffer) EncodeMessage(pb Message) error { | |||
| siz := Size(pb) | |||
| sizVar := SizeVarint(uint64(siz)) | |||
There was a problem hiding this comment.
Shouldn't we be fixing p.Marshal rather than the calling code? If we have to change code that calls Marshal, then I suspect something must be wrong?
There was a problem hiding this comment.
I think you are right. The large change here isn't necessary
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| err = buf.Marshal(m) |
There was a problem hiding this comment.
is buf.Marshal supposed to concatenate into the buffer? I didn't realize that was the expected behavior
There was a problem hiding this comment.
I think this is the case. Using golang/protobuf in this same test does yield a buffer with the same message concatenated into it.
The proto/lib.go does have an explicit Reset() method. Which a client can call when he is done.
https://github.com/golang/protobuf/blob/master/proto/lib.go#L362
| t.Fatal(err) | ||
| } | ||
| bufferBytes := buf.Bytes() | ||
| protoBytes, _ := proto.Marshal(m) |
There was a problem hiding this comment.
You might want to check the ignored err here
| } | ||
| } | ||
|
|
||
| func marshalCheck(t *testing.T, m proto.Message, size int) { |
There was a problem hiding this comment.
I think I liked better the first version of the test (the one in the parent commit)
There was a problem hiding this comment.
Which one did you reference here? I think having a test for encoding twice into the buffer is useful. However, I can make an explicit test for twice encoding.
|
In favor of: #627 |
Compensate the buffer's slice when marshaling into a larger backing buf.