Please reply-all so that others on the mailing list can see...

On Tue, Feb 22, 2011 at 11:51 AM, James Mulvey <james.mul...@gmail.com>wrote:

> 1st issue is relatively easy to work around; I just call clear after
> construction.
> Unfortunately that makes the second bug nasty; since then the outer message
> believes there is no change to the inner message when a modification is made
> to it.
>

This usage pattern is not really supposed to work.  Sub-builders do not work
like sub-messages in Python; they work like sub-messages in C++.  So,
setting a field on a sub-builder is not supposed to mark it present in the
parent message, if it isn't present already.  Basically, when you call
clear(), you're saying "I am not using the sub-builders anymore", and you
have to call getInnerBuilder() again to recreate the inner message.  We
should probably change the code to actually discard all sub-builders on
clear().


> Maybe explaining what I am doing will help:
> I am writing transactional objects composed of an outer wrapper to two
> protobufs; all sets delegate to the delta message; all gets check the delta
> message and then the state.
> Commit basically does state.mergeFrom(delta) and does dela.clear().
> Rollback basically does delta.clear()
>
> So with the second issue the outer object never records the change to the
> sub-object (and thus fails my unit test and use case)
>

I think this would work correctly if you didn't try to clear() immediately
after getInnerBuilder().


>
> I'm still mulling over the 1st issue.. There could be use cases where it is
> useful; but then having a get() mark it as dirty seems a little wrong.
>
> Great work by the way!
>
>
> Sent from my iPhone
>
> On 22 Feb 2011, at 17:09, Kenton Varda <ken...@google.com> wrote:
>
> The first issue is the intended behavior.  It matches C++, where calling
> mutable_inner() instantiates the inner message.  There needs to be a way to
> mark a sub-message "present" without actually setting any of its friends.
>
> The second issue looks genuine, although it seems to me that it would cause
> no behavioral differences, only some wasted instructions.  Is this correct?
>
> 2.4.0a is actually a "final" release.  In fact, it is the successor to
> 2.4.0, which had a build problem.  If there are significant bugs we can do a
> 2.4.0b or 2.4.1 to fix them.
>
> On Tue, Feb 22, 2011 at 6:54 AM, jamesm < <james.mul...@gmail.com>
> james.mul...@gmail.com> wrote:
>
>> I have found two things that look to me like bugs in sub-builder
>> handling; one is in the generated code; the second is in the base
>> class GeneratedMessage.
>>
>> Consider the following:
>>
>> message Inner {
>>  optional int32 somefield = 1;
>> }
>>
>> message Outer{
>>  optional Inner  inner = 1;
>> }
>>
>> Generated code for sub builder is roughtly as follows:
>>      public Inner.Builder getInnerBuilder() {
>>        bitField0_ |= 0x00000004; //wrong - we've not changed it yet
>>        onChanged(); //wrong - we've not changed it yset
>>        return getInnerFieldBuilder().getBuilder();
>>      }
>>
>> The net effect is that calling:
>> outer.getInnerBuilder() marks outer as dirty; which in my view is
>> incorrect
>>
>> The second issue is in GeneratedMessage.Builder:
>>
>>    protected final void onChanged() {
>>      if (isClean && builderParent != null) {
>>        builderParent.markDirty();
>>
>>        // Don't keep dispatching invalidations until build is called
>> again.
>>        isClean = false;
>>      }
>>    }
>> This again seems to be incorrect; it should be as follows:
>>    protected final void onChanged() {
>>      if (isClean && builderParent != null) {
>>        builderParent.markDirty();
>>      }
>>        // Don't keep dispatching invalidations until build is called
>> again.
>>        isClean = false;
>>    }
>>
>> I can see how these two bugs together would pass a basic Junit; but
>> they showed up in some more extensive tests that are part of the
>> system that I'm building.
>>
>> With these changes made we would have the desired effect that only
>> when a modification is made to the sub builder does that builder (and
>> it's parent) get marked as dirty.
>>
>> Please can you confirm that the above are bugs and whether they can be
>> fixed before 2.4.0 final?
>>
>> Thanks
>>
>>
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Protocol Buffers" group.
>> To post to this group, send email to <protobuf@googlegroups.com>
>> protobuf@googlegroups.com.
>> To unsubscribe from this group, send email to
>> <protobuf%2bunsubscr...@googlegroups.com>
>> protobuf+unsubscr...@googlegroups.com.
>> For more options, visit this group at
>> <http://groups.google.com/group/protobuf?hl=en>
>> http://groups.google.com/group/protobuf?hl=en.
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups 
"Protocol Buffers" group.
To post to this group, send email to protobuf@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