Eric Blake <ebl...@redhat.com> writes:
> On 02/17/2016 07:40 AM, Markus Armbruster wrote:
>
>>>>> There's no reason to do two malloc's for an alternate type visiting
>>>>> a QAPI struct; let's just inline the struct directly as the C union
>>>>> branch of the struct.
>>>>>
>
>>> Also, here we pass 'obj'; visit_type_FOO() had to pass '*obj' (again,
>>> because we have one less level of indirection, and 7/13 reduced the
>>> indirection required in visit_type_FOO_fields()).
>>>
>>>> // visit_start_union() + switch dropped
>>>> error_propagate(errp, err);
>>>> err = NULL;
>>>> visit_end_struct(v, &err);
>>>> out:
>>>> error_propagate(errp, err);
>>>> }
>>>>
>>>> Why can we drop visit_start_union() + switch?
>>>
>>> visit_start_union() is dropped because its only purpose was to determine
>>> if the dealloc visitor needs to visit the default branch. When we had a
>>> separate allocation, we did not want to visit the branch if the
>>> discriminator was not successfully parsed, because otherwise we would
>>> dereference NULL. But now that we don't have a second pointer
>>> allocation, we no longer have anything to dealloc, and we can no longer
>>> dereference NULL. Explained better in 12/13, where I delete
>>> visit_start_union() altogether. But maybe I could keep it in this patch
>>> in the meantime, to minimize confusion.
>>
>> Perhaps squashing the two patches together could be less confusing.
>
> Yes, I'm closer to posting v11, and in that version, visit_start_union
> is only dropped in a single patch; and this patch just inlines the
> visit_start_struct() allocation directly into the visit_type_ALT()
> rather than creating a new visit_type_alternate_ALT().
>
>>
>>> Dropped switch, on the other hand, looks to be a genuine bug. Eeek.
>>> That should absolutely be present, and it proves that our testsuite is
>>> not strong enough for not catching me on it.
>>>
>>> And now that you've made me think about it, maybe I have yet another
>>> idea. Right now, we've split the visit of local members into
>>> visit_type_FOO_fields(), while leaving the variant members to be visited
>>> in visit_type_FOO()
>>
>> Yes. I guess that's to support visiting "implicit" structs.
>
> Up to now, we've forbidden the use of one union as a branch of another
> (but allowed a union as a branch of an alternate), so the types passed
> to visit_struct_FOO_fields() never had variants. As part of our generic
> "object" work, I _want_ to support one union as a branch of another (as
> long as we can prove there will be no QMP name collisions); and that's
> another reason why visit_struct_FOO_fields() would need to always visit
> variants if present.
>
>
>> Let me get to this result from a different angle. A "boxed" visit is
>>
>> allocate hook
>> visit the members (common and variant)
>> cleanup hook
>
> Correct, and we have two choices of allocate hook: visit_start_struct()
> [allocate and consume {}, for visit_type_FOO() in general], and
> visit_start_implicit_struct() [allocate, but don't consume {}, for flat
> union branches prior to this series].
>
>>
>> An "unboxed" visit is basically the same without the two hooks.
>
> Done anywhere we don't have a separate C struct [base classes prior to
> this series; and then this series is adding unboxed variant visits
> within flat unions and alternates].
Should work for visiting both "inlined" and "unboxed" members, shouldn't
it?
struct {
A a;
B b
} S;
struct {
S *ps; // boxed member of type S
S s; // unboxed member of type S
A a; B b; // inlined member of type S
}
>>
>> "Implicit" is like unboxed, except the members are inlined rather than
>> wrapped in a JSON object.
>>
>> So the common code to factor out is "visit the members".
>
> Yep, we're on the same wavelength, and it is looking fairly nice for
> what I'm about to post as v11. And I like 'unboxed' better than
> 'is_member':
>
>>>>> - c_type=typ.c_type(),
>>>>> + c_type=typ.c_type(is_member=inline),
>>
>> I don't like the name is_member. The thing we're dealing with here is a
>> member, regardless of the value of inline. I think it's boxed
>> vs. unboxed.
>
> Hmm. I have later patches that add a 'box':true QAPI designation to
> commands and events, to let us do qmp_command(Foo *arg, Error **errp)
> instead of qmp_command(type Foo_member1, type Foo_member2 ..., Error
> **errp) (that is, instead of breaking out each parameter, we pass them
> all boxed behind a single pointer). What we are doing here is in the
> opposite direction - we are taking code that has boxed all the sub-type
> fields behind a single pointer, and unboxing it so that they occur
> inline in the parent type's storage. Works for me; I'm switching to
> 'is_unboxed' as the case for when we want to omit the pointer
> designation during the member declaration.
Better. "Unboxed" isn't tied to "member"; we could choose to unbox
elsewhere, too.
