Done. But it occurs to me that this isn't all that much nicer a solution than just calling getSerializedSize() in writeTo(CodedOutputStream output), except that it only happens for messages with packed fields, rather than all messages. Should this compute only the serialized size of the packed field, so that it doesn't needlessly compute the size of all the unpacked fields? Or does this not really matter? I think all of the wrappers now call getSerializedSize() thanks to r240 (I was tearing my hair out trying to figure out why I could not get a new test which just did ByteArrayOutputStream output = new ByteArrayOutputStream(); message.writeTo(output); would not fail from the open source code, but would if I added the same test to our internal version, until I finally looked at the source). Anyway point being that in most cases getSerializedSize() will have already been called anyway so maybe it doesn't matter too much if we compute the size of the entire message rather than just the packed fields.
On 2009/11/25 03:42:18, kenton wrote: > On Tue, Nov 24, 2009 at 7:40 PM, Kenton Varda <mailto:ken...@google.com> wrote: > > I guess it's technically the case that a packed field always has non-zero > > size, but maybe we should initialize the cached sizes to -1 anyway to make > > it more clear when they haven't been initialized? > That was a confusing sentence. Change "when they haven't been initialized" > to "when getSerializedSize() hasn't been called". > > > > On Tue, Nov 24, 2009 at 7:04 PM, <mailto:jas...@google.com> wrote: > > > >> Reviewers: kenton, > >> > >> Description: > >> Fix Issue 136: the memoized serialized size for packed fields may not > >> be properly set. writeTo() may be invoked without a call to > >> getSerializedSize(), so the generated serialization methods would > >> write a length of 0 for non-empty packed fields. Since the object > >> is immutable, we know that the memoized size is 0 or set to the > >> correct size. In the generated code, check to see if the memoized > >> size is 0, and if so, call getSerializedSize() before actually > >> emitting the field. > >> > >> Tested: new unittest case in WireFormatTest.java now passes > >> > >> > >> > >> Please review this at http://codereview.appspot.com/157169 > >> > >> Affected files: > >> M java/src/test/java/com/google/protobuf/WireFormatTest.java > >> M src/google/protobuf/compiler/java/java_primitive_field.cc > >> > >> > >> Index: java/src/test/java/com/google/protobuf/WireFormatTest.java > >> =================================================================== > >> --- java/src/test/java/com/google/protobuf/WireFormatTest.java (revision > >> 246) > >> +++ java/src/test/java/com/google/protobuf/WireFormatTest.java (working > >> copy) > >> @@ -102,6 +102,28 @@ > >> assertEquals(rawBytes, rawBytes2); > >> } > >> > >> + public void testSerializationPackedWithoutGetSerializedSize() > >> + throws Exception { > >> + // Write directly to an OutputStream, without invoking > >> getSerializedSize() > >> + // This used to be a bug where the size of a packed field was > >> incorrect, > >> + // since getSerializedSize() was never invoked. > >> + TestPackedTypes message = TestUtil.getPackedSet(); > >> + > >> + // Directly construct a CodedOutputStream around the actual > >> OutputStream, > >> + // because writeTo() now invokes getSerializedSize(); > >> + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); > >> + CodedOutputStream codedOutput = > >> CodedOutputStream.newInstance(outputStream); > >> + > >> + message.writeTo(codedOutput); > >> + > >> + codedOutput.flush(); > >> + > >> + TestPackedTypes message2 = TestPackedTypes.parseFrom( > >> + outputStream.toByteArray()); > >> + > >> + TestUtil.assertPackedFieldsSet(message2); > >> + } > >> + > >> public void testSerializeExtensionsLite() throws Exception { > >> // TestAllTypes and TestAllExtensions should have compatible wire > >> formats, > >> // so if we serialize a TestAllExtensions then parse it as > >> TestAllTypes > >> Index: src/google/protobuf/compiler/java/java_primitive_field.cc > >> =================================================================== > >> --- src/google/protobuf/compiler/java/java_primitive_field.cc (revision > >> 246) > >> +++ src/google/protobuf/compiler/java/java_primitive_field.cc (working > >> copy) > >> @@ -384,8 +384,17 @@ > >> void RepeatedPrimitiveFieldGenerator:: > >> GenerateSerializationCode(io::Printer* printer) const { > >> if (descriptor_->options().packed()) { > >> + // writeTo() may be called without getSerializedSize() ever having > >> been > >> + // called, so we may need to compute the length of the packed data. > >> Since > >> + // the object is immutable, we know that the memoized size is either > >> set to > >> + // 0 (via object initialization) or else it has the correct size from > >> a > >> + // previous getSerializedSize() call. If the field is not empty and > >> the size > >> + // has not yet been computed, just call getSerializedSize() > >> printer->Print(variables_, > >> "if (get$capitalized_name$List().size() > 0) {\n" > >> + " if ($name$MemoizedSerializedSize == 0) {\n" > >> + " getSerializedSize();\n" > >> + " }\n" > >> " output.writeRawVarint32($tag$);\n" > >> " output.writeRawVarint32($name$MemoizedSerializedSize);\n" > >> "}\n" > >> > >> > >> > > http://codereview.appspot.com/157169 -- You received this message because you are subscribed to the Google Groups "Protocol Buffers" group. To post to this group, send email to proto...@googlegroups.com. To unsubscribe from this group, send email to protobuf+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/protobuf?hl=en.
