On 03/10/2016 11:50 AM, Markus Armbruster wrote: > Eric Blake <ebl...@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
signature.asc
Description: OpenPGP digital signature