Re: [Qemu-devel] [PATCH v11 12/15] qapi: Don't box struct branch of alternate

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 02/18/2016 09:43 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> 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.
>>>
>>>
>>> meanwhile, in qapi-visit.h, we trade the previous visit_type_FOO(obj)
>> 
>> Meanwhile
>> 
>>> (which allocates a pointer, handles a layer of {} from the JSON stream,
>>> and visits the fields)
>
> Maybe:
>
> (which first calls visit_start_struct(v, name, obj, size, &err) to
> allocate a pointer and handle a layer of {} from the JSON stream, then
> visits the fields)
>
>>>                         with an inline call to visit_type_FOO(NULL) (to
>>> consume the {} without allocation) and a direct visit of the fields:
>> 
>> I don't see a call to visit_type_FOO(NULL).
>
> Whoops, wrong function name.  I meant:
>
> visit_start_struct(v, name, NULL, 0, &err).
>                             ^^^^
> Especially useful if you take my earlier text suggestion to point out
> the difference in the visit_start_struct() calls as being the
> intentional so that we continue to consume {} but now opt out of a
> second allocation.

Better, but the sentence is long enough to confuse even a German.  What
about:

    The corresponding spot in qapi-visit.c calls visit_type_FOO(), which
    first calls visit_start_struct() to allocate or deallocate the member
    and handle a layer of {} from the JSON stream, then visits the
    members.  To peel off the indirection and the memory management that
    comes with it, we inline this call, then suppress allocation /
    deallocation by passing NULL to visit_start_struct(), and adjust the
    member visit:

>> 
>> Suggest not to abbreviate argument lists, it's mildly confusing.
>
> Sure. Are we still at a point that you could touch up the commit message
> without a patch respin?

Looks like it so far.

>> 
>> Patch looks good.
>>