You're right, we might as well always call it. On Wed, Nov 25, 2009 at 9:08 AM, <jas...@google.com> wrote:
> 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.
