Re: [Qemu-devel] [PATCH v5 06/14] qapi-event: Slightly shrink generated code

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3203 bytes --]

On 03/10/2016 11:50 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> Slightly rearrange the code in gen_event_send() for less generated
>> output, by initializing 'emit' sooner, deferring an assertion
>> to qdict_put_obj, and dropping a now-unused 'obj' local variable.
>>
>> While at it, document a FIXME related to the potential for a
>> compiler naming collision - if the user ever creates a QAPI event
>> whose name matches 'errp' or one of our local variables (like
>> 'emit'), we'll have to revisit how we generate functions to
>> avoid the problem.
>>

> 
> We're not "deferring an assertion to qdict_put_obj()", we're dropping a
> dead one: qmp_output_get_qobject() never returns null.

Oh, good point; I can improve the commit message.

> 
> I figure the assertion dates back to the time when it still did.  Back
> then, getting null here meant we screwed up.
> 
> I just searched the code for similarly dead assertions.  Found one in
> qapi_clone_InputEvent(), and serveral more in test-qmp-output-visitor.c.

Speaking of that, I have a patch pending (but not yet posted) that adds
a clone visitor, so that we don't need qapi_clone_InputEvent() (it's
rather wasteful to convert into and back out of QObject when you can
just directly clone).

> 
> There's also an error return in qapi_copy_SocketAddress().  Useless?

And that's the other hand-rolled clone that also gets nuked by my patch.
 Some obvious copy-and-paste between the two.

> Should check for qnull instead?

Not necessary; we can't return qnull unless we visit nothing (or, when
my visit_type_null() lands, if we explicitly ask for it), but these
callers are visiting something that is not null.


>>  %(proto)s
>>  {
>>      QDict *qmp;
>>      Error *err = NULL;
>> -    QMPEventFuncEmit emit;
>> +    QMPEventFuncEmit emit = qmp_event_get_func_emit();
>>  ''',
>>                  proto=gen_event_send_proto(name, arg_type))
>>
>> @@ -43,16 +49,13 @@ def gen_event_send(name, arg_type):
>>          ret += mcgen('''
>>      QmpOutputVisitor *qov;
>>      Visitor *v;
>> -    QObject *obj;
>> -
> 
> Please keep the blank line here...
> 
>>  ''')
>>
>>      ret += mcgen('''
>> -    emit = qmp_event_get_func_emit();
>> +
> 
> ... instead of adding it here.

Except that the next patch added one more declaration after Visitor *v,
but not in direct text, where keeping the blank line unmoved would
require splitting the mcgen() call into two parts.  Or I could do ret +=
'\n'.

> 
>>      if (!emit) {
>>          return;
>>      }
>> -
> 
> Let's keep this one.

Okay.

> 
>>      qmp = qmp_event_build_dict("%(name)s");
>>
>>  ''',
>> @@ -76,11 +79,7 @@ out_obj:
>>      if (err) {
>>          goto out;
>>      }
>> -
>> -    obj = qmp_output_get_qobject(qov);
>> -    g_assert(obj);
>> -
>> -    qdict_put_obj(qmp, "data", obj);
>> +    qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov));
>>  ''')
>>
>>      ret += mcgen('''
> 
> Small improvements are welcome, too :)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]