Re: [Qemu-devel] [PATCH v2 11/19] qapi: Add type.is_empty() helper

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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

I suspect what's needed is a clearer commit message a fresher mind
reading it.

>>> 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.

Unless allowing unions makes generators nicer, I'll want to see a
compelling use case.

Can you give me an idea what .is_empty() does for *this* patch series?
Wait, you did: "will help fix a bug [...] in the next patch".

Perhaps a slight rearrangement would make things easier to grok: start
with the bug fix, and introduce .is_empty() there.  In the next patch,
say "In these places, the intent is to test for empty, but the actual
code exploits that there can be no variants.  Using .is_empty() instead
is a bit clearer and a bit more robust."

>>> +++ 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.

Also needs a use case.

>> Haven't reviewed for completeness.