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?
On Tue, Nov 24, 2009 at 7:04 PM, <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" > > > -- 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.
