[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 11/19] qapi: Add type.is_empty() helper
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 11/19] qapi: Add type.is_empty() helper |
Date: |
Wed, 2 Mar 2016 13:16:49 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
On 03/02/2016 12:04 PM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
>
>> And use it in qapi-types and qapi-event. Down the road, we may
>> want to lift our artificial restriction of no variants at the
>> top level of an event, at which point, inlining our check for
>> whether members is empty will no longer be sufficient, but
>> adding a check for variants adds verbosity; in the meantime,
>> add some asserts in places where we don't handle variants.
>
> Perhaps I'm just running out of steam for today, but I've read this
> twice, and still don't get why adding these assertions goes in the same
> patch as adding the helper, or what it has to do with events.
And yet it was the review on the earlier posting that caused me to add
asserts; maybe re-reading that thread will help refresh memory, and spur
an idea for how to better express it in the commit message:
https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04726.html
>
>> More immediately, the new .is_empty() helper will help fix a bug
>> in qapi-visit in the next patch, where the generator did not
>> handle an explicit empty type in the same was as a missing type.
>
> same way
[Ever wonder if I intentionally stick in a typo, just to see who will
notice? Or maybe it really was a slip of the finger...]
>> +++ b/scripts/qapi-event.py
>> @@ -39,7 +39,7 @@ def gen_event_send(name, arg_type):
>> ''',
>> proto=gen_event_send_proto(name, arg_type))
>>
>> - if arg_type and arg_type.members:
>> + if arg_type and not arg_type.is_empty():
>> ret += mcgen('''
>> QmpOutputVisitor *qov;
>> Visitor *v;
>
> Oh, you don't just add a helper, you actually *change* the condition!
> Perhaps the commit message would be easier to understand if it explained
> that first.
The old condition:
arg_type and arg_type.members
New condition:
arg_type and (arg_type.members or arg_type.variants)
But we know there are no variants, since unions cannot (yet) be passed
as event 'data', so the condition is the same effect now, and
future-proofing for a future patch when I do allow unions in events.
>> +++ b/scripts/qapi-types.py
>> @@ -90,7 +90,7 @@ struct %(c_name)s {
>> # potential issues with attempting to malloc space for zero-length
>> # structs in C, and also incompatibility with C++ (where an empty
>> # struct is size 1).
>> - if not (base and base.members) and not members and not variants:
>> + if (not base or base.is_empty()) and not members and not variants:
>> ret += mcgen('''
>> char qapi_dummy_for_empty_struct;
>> ''')
>
> I figure the case for the helper based on this patch alone is making the
> code a bit more future-proof. Suggest you try to explain that in your
> commit message, including against what future change exactly you're
> proofing the code.
And here, bases cannot (yet) have variants, but that's also on my plate
of things I'd like to support in the future.
>
> Haven't reviewed for completeness.
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature