Done. I kept the change to initialize the memoized size of packed fields
to -1, just for consistency with the message's size.

On 2009/11/25 20:51:52, kenton wrote:
> You're right, we might as well always call it.

> On Wed, Nov 25, 2009 at 9:08 AM, <mailto: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
> >




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.


Reply via email to