patch to improve Swap and add RemoveLast

[kenton, 2009-03-16 23:22:11]

Hi Scott,

Thanks for doing this!  There were some other users recently who asked for
functionality like this.  Nitpicky comments below.

I think I'd rather not add Swap() to the Message base interface.  The
requirements for using it are different from other, seemingly similar methods,
like MergeFrom() and CopyFrom().  For those methods, the other message can be of
any type that has the same Descriptor, but for Swap(), the other message must be
of the exact same subclass.  If you know that two messages are of the same
subclass, it seems like you might as well down-cast them and call Swap()
directly.

What do you think?

A bunch of my comments below no longer apply in this case, since the code
involved would be removed.

http://codereview.appspot.com/27053/diff/1/15
File src/google/protobuf/extension_set.cc (right):

http://codereview.appspot.com/27053/diff/1/15#newcode410
Line 410: extension->repeated_int32_value->RemoveLast(); break;
I'd prefer if the breaks were on their own lines.

http://codereview.appspot.com/27053/diff/1/15#newcode429
Line 429: default:
Please just omit the default case.  That way, GCC will produce a compiler
warning if one of the enums values is missing (and I compile with -Werror).

http://codereview.appspot.com/27053/diff/1/15#newcode459
Line 459: default:
Same here.

http://codereview.appspot.com/27053/diff/1/6
File src/google/protobuf/generated_message_reflection.cc (right):

http://codereview.appspot.com/27053/diff/1/6#newcode258
Line 258: {
Put the brace at the end of the previous line to match Google style.

http://codereview.appspot.com/27053/diff/1/6#newcode270
Line 270: //if (GetRaw<GenericRepeatedField>(message, field).GenericSize() > 0)
{
Remove commented code.

http://codereview.appspot.com/27053/diff/1/6#newcode273
Line 273: MutableRaw<GenericRepeatedField>(message1,
field)->GenericSwap(MutableRaw<GenericRepeatedField>(message2, field));
Please wrap code at 80 characters.

http://codereview.appspot.com/27053/diff/1/6#newcode278
Line 278: // TODO(ses): could be more efficient if it cleared field directly
rather than ClearField
Use either your name or your full e-mail address so that people know you aren't
@google.com.

http://codereview.appspot.com/27053/diff/1/6#newcode328
Line 328: /*
Why is all this commented out?

http://codereview.appspot.com/27053/diff/1/13
File src/google/protobuf/generated_message_reflection.h (right):

http://codereview.appspot.com/27053/diff/1/13#newcode143
Line 143: void Swap(Message* message, const FieldDescriptor* field, int index1,
int index2) const;
80 chars

http://codereview.appspot.com/27053/diff/1/5
File src/google/protobuf/generated_message_reflection_unittest.cc (right):

http://codereview.appspot.com/27053/diff/1/5#newcode164
Line 164: EXPECT_EQ(size-j-1, reflection->FieldSize(message, field));
You should probably also check that the correct value was removed.

Maybe instead of iterating through all the fields, you could instead just go
through one field of each CppType, do RemoveLast() on it via the reflection
interface, and then check the contents via the normal interface?

It might even make sense to add methods to TestUtil::ReflectionTester like
"RemoveLastRepeateds()" and "SwapRepeateds()", and methods to TestUtil like
"ExpectLastRepeatedsRemoved()" and "ExpectLastRepeatedsSwapped()".  Then you can
call those from here.  This way you could follow the patterns in that file and
you would not have to repeat as much work for regular fields vs. extensions.

http://codereview.appspot.com/27053/diff/1/5#newcode199
Line 199: for (int i=0; i<output.size(); ++i) {
Similarly here, I think you should target a field of each CppType by name rather
than iterate through them (possibly by adding code to test_util.cc).  That way
you can easily test that the results are correct.  Currently it looks like you
are only calling each method, not verifying results.  (You are verifying that a
double-swap returns the message to its original state but this would pass even
if Swap() did nothing.)

http://codereview.appspot.com/27053/diff/1/5#newcode232
Line 232: reflection->Swap(&message, field, 0, 1);
This belongs in a separate test.

http://codereview.appspot.com/27053/diff/1/10
File src/google/protobuf/message.h (right):

http://codereview.appspot.com/27053/diff/1/10#newcode179
Line 179: // Swap the contents of the message with another message.
Please clarify that the other message must have the same Descriptor *and* be of
exactly the same class.

http://codereview.appspot.com/27053/diff/1/10#newcode491
Line 491: virtual void Swap(Message* message,
Let's call this SwapElements() instead.  I don't like overloading methods with
very different meanings.

http://codereview.appspot.com/27053/diff/1/10#newcode503
Line 503: 
Remove this random newline that you added.

http://codereview.appspot.com/27053/diff/1/4
File src/google/protobuf/reflection_ops.cc (right):

http://codereview.appspot.com/27053/diff/1/4#newcode130
Line 130: /*
Please remove this code rather than commenting it out.

http://codereview.appspot.com/27053/diff/1/7
File src/google/protobuf/reflection_ops.h (right):

http://codereview.appspot.com/27053/diff/1/7#newcode61
Line 61: static void Swap(Message* message1, Message* message2);
I don't think there's any reason to add a Swap() method here when it's already
an explicit method of the Message and Reflection interfaces.  It doesn't really
add much.

http://codereview.appspot.com/27053/diff/1/12
File src/google/protobuf/reflection_ops_unittest.cc (right):

http://codereview.appspot.com/27053/diff/1/12#newcode275
Line 275: TEST(ReflectionOpsTest, RemoveLastUnknown) {
The contents of this test appear to be the same as ClearUnknown.  Remove it?

http://codereview.appspot.com/27053/diff/1/3
File src/google/protobuf/repeated_field.h (right):

http://codereview.appspot.com/27053/diff/1/3#newcode142
Line 142: void Swap(int index1, int index2);
Rename to SwapElements().

http://codereview.appspot.com/27053/diff/1/3#newcode415
Line 415: std::swap(*Mutable(index1), *Mutable(index2));
Don't use the std:: prefix here -- we want to use Koenig lookup for swap(). 
Also, we use "using namespace std" anyway.

http://codereview.appspot.com/27053/diff/1/3#newcode473
Line 473: // TODO(ses): Can this be arranged somehow without the dynamic_cast?
You should use down_cast instead of dynamic_cast.  (down_cast is defined in
stubs/common.h.  It is dynamic_cast with an assert in debug mode and static_cast
in opt mode.)

http://codereview.appspot.com/27053/diff/1/3#newcode664
Line 664: std::swap(*Mutable(index1), *Mutable(index2));
Just swap the pointers here, not the contents:

  swap(elements_[index1], elements_[index2]);

http://codereview.appspot.com/27053/diff/1/3#newcode668
Line 668: // for UnknownFieldSet in unknown_field_set.h.
(And then you don't need this specialization anymore.)

http://codereview.appspot.com/27053/diff/1/3#newcode674
Line 674: void RepeatedPtrField<Message>::Swap(int index1, int index2);
(Nor this one.)

http://codereview.appspot.com/27053/diff/1/3#newcode768
Line 768: // TODO(ses): Can this be arranged somehow without the dynamic_cast?
down_cast

[ses4j, 2009-06-16 04:11:41]

I left Swap() in the Message base interface, but addressed all other changes. 
If you want me to remove it from Message, I'm relatively indifferent.  Anyway,
at long last here's the revised patch. Sorry it took a while, my wife got
pregnant, I moved to a house, my (real work) project got cancelled, ...

http://codereview.appspot.com/27053/diff/1/15
File src/google/protobuf/extension_set.cc (right):

http://codereview.appspot.com/27053/diff/1/15#newcode410
Line 410: extension->repeated_int32_value->RemoveLast(); break;
On 2009/03/16 23:22:12, kenton wrote:
> I'd prefer if the breaks were on their own lines.

Done.

http://codereview.appspot.com/27053/diff/1/15#newcode429
Line 429: default:
On 2009/03/16 23:22:12, kenton wrote:
> Please just omit the default case.  That way, GCC will produce a compiler
> warning if one of the enums values is missing (and I compile with -Werror).

Done.

http://codereview.appspot.com/27053/diff/1/15#newcode459
Line 459: default:
On 2009/03/16 23:22:12, kenton wrote:
> Same here.

Done.

http://codereview.appspot.com/27053/diff/1/6
File src/google/protobuf/generated_message_reflection.cc (right):

http://codereview.appspot.com/27053/diff/1/6#newcode258
Line 258: {
On 2009/03/16 23:22:12, kenton wrote:
> Put the brace at the end of the previous line to match Google style.

Done.

http://codereview.appspot.com/27053/diff/1/6#newcode270
Line 270: //if (GetRaw<GenericRepeatedField>(message, field).GenericSize() > 0)
{
On 2009/03/16 23:22:12, kenton wrote:
> Remove commented code.

Done.

http://codereview.appspot.com/27053/diff/1/6#newcode273
Line 273: MutableRaw<GenericRepeatedField>(message1,
field)->GenericSwap(MutableRaw<GenericRepeatedField>(message2, field));
On 2009/03/16 23:22:12, kenton wrote:
> Please wrap code at 80 characters.

Done.

http://codereview.appspot.com/27053/diff/1/6#newcode278
Line 278: // TODO(ses): could be more efficient if it cleared field directly
rather than ClearField
On 2009/03/16 23:22:12, kenton wrote:
> Use either your name or your full e-mail address so that people know you
aren't
> @google.com.

Done.

http://codereview.appspot.com/27053/diff/1/6#newcode328
Line 328: /*
On 2009/03/16 23:22:12, kenton wrote:
> Why is all this commented out?

oops.  cleared.

http://codereview.appspot.com/27053/diff/1/13
File src/google/protobuf/generated_message_reflection.h (right):

http://codereview.appspot.com/27053/diff/1/13#newcode143
Line 143: void Swap(Message* message, const FieldDescriptor* field, int index1,
int index2) const;
On 2009/03/16 23:22:12, kenton wrote:
> 80 chars

Done.

http://codereview.appspot.com/27053/diff/1/5
File src/google/protobuf/generated_message_reflection_unittest.cc (right):

http://codereview.appspot.com/27053/diff/1/5#newcode164
Line 164: EXPECT_EQ(size-j-1, reflection->FieldSize(message, field));
On 2009/03/16 23:22:12, kenton wrote:
> You should probably also check that the correct value was removed.
> 
> Maybe instead of iterating through all the fields, you could instead just go
> through one field of each CppType, do RemoveLast() on it via the reflection
> interface, and then check the contents via the normal interface?
> 
> It might even make sense to add methods to TestUtil::ReflectionTester like
> "RemoveLastRepeateds()" and "SwapRepeateds()", and methods to TestUtil like
> "ExpectLastRepeatedsRemoved()" and "ExpectLastRepeatedsSwapped()".  Then you
can
> call those from here.  This way you could follow the patterns in that file and
> you would not have to repeat as much work for regular fields vs. extensions.

I did as (I think) you suggested, but I don't know how much copy/pasting I
saved.  A little, but I still needed an Expect* and an ExpectExtensions*.

http://codereview.appspot.com/27053/diff/1/5#newcode199
Line 199: for (int i=0; i<output.size(); ++i) {
On 2009/03/16 23:22:12, kenton wrote:
> Similarly here, I think you should target a field of each CppType by name
rather
> than iterate through them (possibly by adding code to test_util.cc).  That way
> you can easily test that the results are correct.  Currently it looks like you
> are only calling each method, not verifying results.  (You are verifying that
a
> double-swap returns the message to its original state but this would pass even
> if Swap() did nothing.)

No, it tests the message both ways, after one swap and after two swaps
(returning to normalcy).  See the TEST_SWAP_REPEATED_ELEMENTS(), testing against
the messageCopy.  I rejiggered it anyway, to be less sneaky and more like the
rest of the tests...

http://codereview.appspot.com/27053/diff/1/5#newcode232
Line 232: reflection->Swap(&message, field, 0, 1);
On 2009/03/16 23:22:12, kenton wrote:
> This belongs in a separate test.

I left it in the same test but they're MUCH shorter now thanks to the TestUtil
move.  Let me know if you want me to break it out anyway...

http://codereview.appspot.com/27053/diff/1/10
File src/google/protobuf/message.h (right):

http://codereview.appspot.com/27053/diff/1/10#newcode179
Line 179: // Swap the contents of the message with another message.
On 2009/03/16 23:22:12, kenton wrote:
> Please clarify that the other message must have the same Descriptor *and* be
of
> exactly the same class.

Done.

http://codereview.appspot.com/27053/diff/1/10#newcode491
Line 491: virtual void Swap(Message* message,
On 2009/03/16 23:22:12, kenton wrote:
> Let's call this SwapElements() instead.  I don't like overloading methods with
> very different meanings.

Done.

http://codereview.appspot.com/27053/diff/1/10#newcode503
Line 503: 
On 2009/03/16 23:22:12, kenton wrote:
> Remove this random newline that you added.

Done.

http://codereview.appspot.com/27053/diff/1/4
File src/google/protobuf/reflection_ops.cc (right):

http://codereview.appspot.com/27053/diff/1/4#newcode130
Line 130: /*
On 2009/03/16 23:22:12, kenton wrote:
> Please remove this code rather than commenting it out.

Done.

http://codereview.appspot.com/27053/diff/1/7
File src/google/protobuf/reflection_ops.h (right):

http://codereview.appspot.com/27053/diff/1/7#newcode61
Line 61: static void Swap(Message* message1, Message* message2);
On 2009/03/16 23:22:12, kenton wrote:
> I don't think there's any reason to add a Swap() method here when it's already
> an explicit method of the Message and Reflection interfaces.  It doesn't
really
> add much.

I don't really understand - I thought I was just following the pattern laid down
by Clear(), IsInitialized(), etc. in this class.  How is Swap different?

http://codereview.appspot.com/27053/diff/1/12
File src/google/protobuf/reflection_ops_unittest.cc (right):

http://codereview.appspot.com/27053/diff/1/12#newcode275
Line 275: TEST(ReflectionOpsTest, RemoveLastUnknown) {
On 2009/03/16 23:22:12, kenton wrote:
> The contents of this test appear to be the same as ClearUnknown.  Remove it?

Done.

http://codereview.appspot.com/27053/diff/1/3
File src/google/protobuf/repeated_field.h (right):

http://codereview.appspot.com/27053/diff/1/3#newcode142
Line 142: void Swap(int index1, int index2);
On 2009/03/16 23:22:12, kenton wrote:
> Rename to SwapElements().

Done.

http://codereview.appspot.com/27053/diff/1/3#newcode415
Line 415: std::swap(*Mutable(index1), *Mutable(index2));
On 2009/03/16 23:22:12, kenton wrote:
> Don't use the std:: prefix here -- we want to use Koenig lookup for swap(). 
> Also, we use "using namespace std" anyway.

Done.

http://codereview.appspot.com/27053/diff/1/3#newcode473
Line 473: // TODO(ses): Can this be arranged somehow without the dynamic_cast?
On 2009/03/16 23:22:12, kenton wrote:
> You should use down_cast instead of dynamic_cast.  (down_cast is defined in
> stubs/common.h.  It is dynamic_cast with an assert in debug mode and
static_cast
> in opt mode.)

Done.

http://codereview.appspot.com/27053/diff/1/3#newcode664
Line 664: std::swap(*Mutable(index1), *Mutable(index2));
On 2009/03/16 23:22:12, kenton wrote:
> Just swap the pointers here, not the contents:
> 
>   swap(elements_[index1], elements_[index2]);

Done.

http://codereview.appspot.com/27053/diff/1/3#newcode668
Line 668: // for UnknownFieldSet in unknown_field_set.h.
On 2009/03/16 23:22:12, kenton wrote:
> (And then you don't need this specialization anymore.)

Done.

http://codereview.appspot.com/27053/diff/1/3#newcode674
Line 674: void RepeatedPtrField<Message>::Swap(int index1, int index2);
On 2009/03/16 23:22:12, kenton wrote:
> (Nor this one.)

Done.

http://codereview.appspot.com/27053/diff/1/3#newcode768
Line 768: // TODO(ses): Can this be arranged somehow without the dynamic_cast?
On 2009/03/16 23:22:12, kenton wrote:
> down_cast

Done.

[kenton, 2009-06-19 21:14:39]

Looking pretty good.

I'd still prefer not to add Swap() to the Message interface.  Unlike CopyFrom()
and MergeFrom(), Swap() only works if the two types are the exact same class,
not if they are different classes with the same descriptor, which makes it much
less useful to have a generic Swap().

http://codereview.appspot.com/27053/diff/2002/3001
File CHANGES.txt (right):

http://codereview.appspot.com/27053/diff/2002/3001#newcode89
Line 89: * Fixed bug where Message.Swap(Message) was only implemented for
Please make sure to update these to reflect the latest version of the patch. 
Also, since they didn't end up in 2.1.0, please create a new section at the top
of the file for 2.1.1 and put these under it.

http://codereview.appspot.com/27053/diff/2002/3016
File src/google/protobuf/extension_set.cc (right):

http://codereview.appspot.com/27053/diff/2002/3016#newcode503
Line 503: // TODO(Scott Stafford): DCHECK to see if it's repeated?
Yes, please.

http://codereview.appspot.com/27053/diff/2002/3016#newcode543
Line 543: // TODO(Scott Stafford): DCHECK to see if it's repeated?
yep

http://codereview.appspot.com/27053/diff/2002/3005
File src/google/protobuf/generated_message_reflection.cc (right):

http://codereview.appspot.com/27053/diff/2002/3005#newcode277
Line 277: GOOGLE_CHECK_EQ(message1->GetDescriptor(), descriptor_)
This isn't an adequate check.  You actually need to verify that the message is
of the exact right class, not just that it has the same descriptor.  You could
do this by:

GOOGLE_CHECK_EQ(message1->GetReflection(), this)

http://codereview.appspot.com/27053/diff/2002/3005#newcode301
Line 301: ClearField(message1, field); \
The style used in other macro definitions in this file is to make sure all the
line-ending backslashes line up.  Please do that here too.

http://codereview.appspot.com/27053/diff/2002/3003
File src/google/protobuf/reflection_ops.cc (right):

http://codereview.appspot.com/27053/diff/2002/3003#newcode126
Line 126: GOOGLE_CHECK_EQ(message2->GetDescriptor(), descriptor)
This seems redundant since the reflection object will do its own checking.

http://codereview.appspot.com/27053/diff/2002/3010
File src/google/protobuf/repeated_field.cc (right):

http://codereview.appspot.com/27053/diff/2002/3010#newcode61
Line 61: /*
Remove commented code.  (I think you can remove this file entirely from the
patch.)

http://codereview.appspot.com/27053/diff/2002/3007
File src/google/protobuf/test_util.h (right):

http://codereview.appspot.com/27053/diff/2002/3007#newcode101
Line 101: static void ExpectLastRepeatedExtensionsRemoved(const
unittest::TestAllExtensions& message);
Please wrap this line so it is less than 80 chars.

http://codereview.appspot.com/27053/diff/2002/3007#newcode103
Line 103: // Check that all repeated fields have had their first and last
elements swapped.
80 chars

http://codereview.appspot.com/27053/diff/2002/3007#newcode105
Line 105: static void ExpectRepeatedExtensionsSwapped(const
unittest::TestAllExtensions& message);
80 chars

[ses4j, 2009-06-25 04:37:43]

Addressed suggestions from round 2, removing Message::Swap.  Also moved unit
tests to generated_message_reflection_unittest.cc.

http://codereview.appspot.com/27053/diff/2002/3001
File CHANGES.txt (right):

http://codereview.appspot.com/27053/diff/2002/3001#newcode89
Line 89: * Fixed bug where Message.Swap(Message) was only implemented for
On 2009/06/19 21:14:40, kenton wrote:
> Please make sure to update these to reflect the latest version of the patch. 
> Also, since they didn't end up in 2.1.0, please create a new section at the
top
> of the file for 2.1.1 and put these under it.

Done.

http://codereview.appspot.com/27053/diff/2002/3016
File src/google/protobuf/extension_set.cc (right):

http://codereview.appspot.com/27053/diff/2002/3016#newcode503
Line 503: // TODO(Scott Stafford): DCHECK to see if it's repeated?
On 2009/06/19 21:14:40, kenton wrote:
> Yes, please.

Done.

http://codereview.appspot.com/27053/diff/2002/3016#newcode543
Line 543: // TODO(Scott Stafford): DCHECK to see if it's repeated?
On 2009/06/19 21:14:40, kenton wrote:
> yep

Done.

http://codereview.appspot.com/27053/diff/2002/3005
File src/google/protobuf/generated_message_reflection.cc (right):

http://codereview.appspot.com/27053/diff/2002/3005#newcode277
Line 277: GOOGLE_CHECK_EQ(message1->GetDescriptor(), descriptor_)
On 2009/06/19 21:14:40, kenton wrote:
> This isn't an adequate check.  You actually need to verify that the message is
> of the exact right class, not just that it has the same descriptor.  You could
> do this by:
> 
> GOOGLE_CHECK_EQ(message1->GetReflection(), this)

Done.

http://codereview.appspot.com/27053/diff/2002/3005#newcode301
Line 301: ClearField(message1, field); \
On 2009/06/19 21:14:40, kenton wrote:
> The style used in other macro definitions in this file is to make sure all the
> line-ending backslashes line up.  Please do that here too.

Done.

http://codereview.appspot.com/27053/diff/2002/3003
File src/google/protobuf/reflection_ops.cc (right):

http://codereview.appspot.com/27053/diff/2002/3003#newcode126
Line 126: GOOGLE_CHECK_EQ(message2->GetDescriptor(), descriptor)
On 2009/06/19 21:14:40, kenton wrote:
> This seems redundant since the reflection object will do its own checking.

Removed entire method since generated code just calls Reflection::Swap directly.

http://codereview.appspot.com/27053/diff/2002/3010
File src/google/protobuf/repeated_field.cc (right):

http://codereview.appspot.com/27053/diff/2002/3010#newcode61
Line 61: /*
On 2009/06/19 21:14:40, kenton wrote:
> Remove commented code.  (I think you can remove this file entirely from the
> patch.)

Done.

http://codereview.appspot.com/27053/diff/2002/3007
File src/google/protobuf/test_util.h (right):

http://codereview.appspot.com/27053/diff/2002/3007#newcode101
Line 101: static void ExpectLastRepeatedExtensionsRemoved(const
unittest::TestAllExtensions& message);
On 2009/06/19 21:14:40, kenton wrote:
> Please wrap this line so it is less than 80 chars.

Done.

http://codereview.appspot.com/27053/diff/2002/3007#newcode103
Line 103: // Check that all repeated fields have had their first and last
elements swapped.
On 2009/06/19 21:14:40, kenton wrote:
> 80 chars

Done.

http://codereview.appspot.com/27053/diff/2002/3007#newcode105
Line 105: static void ExpectRepeatedExtensionsSwapped(const
unittest::TestAllExtensions& message);
On 2009/06/19 21:14:40, kenton wrote:
> 80 chars

Done.

[kenton, 2009-06-25 19:06:02]

I went ahead and make Reflection::Swap() a little faster by swapping has_bits
directly, and then swapping the field values without looking at their has bits. 
I also added you to CONTRIBUTORS.txt.

Committed as rev 155.

Thanks!