Eric Blake <ebl...@redhat.com> writes: > On 03/08/2016 08:10 AM, Markus Armbruster wrote: >> Eric Blake <ebl...@redhat.com> writes: >> >>> Rather than generate inline per-member visits, take advantage >>> of the 'visit_type_FOO_members()' function for both event and >>> command marshalling. This is possible now that implicit >>> structs can be visited like any other. >>> >>> Generated code shrinks accordingly; events initialize a struct >>> based on parameters, such as: >>> > >>> >>> And command marshalling generates call arguments from a stack-allocated >>> struct: >> >> I see qmp-marshal.c shrink from ~5700 lines to ~4300. Neat! > > Yeah, it nicely balances out the .h getting so much larger, except that > the .h gets parsed a lot more by the compiler. > > >>> >>> +# Declare and initialize an object 'qapi' using parameters from >>> gen_params() >>> +def gen_struct_init(typ): >> >> It's not just a "struct init", it's a variable declaration with an >> initializer. gen_param_var()? >> >> Name the variable param rather than qapi? > > Sure, I'm not tied to a specific name. I will point out that we have a > potential collision point - any local variable we create here can > collide with members of the QAPI struct passed to the event. We haven't > hit the collision on any events we've created so far, and it's easy to > rename our local variables at such time if we do run into the collision > down the road, so I won't worry about it now.
This patch actually fixes a similar issue in the qmp_marshal_FOO() functions. To keep ignoring it in the qapi_event_send_BAR() functions is okay. It's fairly easy to fix now, though: split them into two, so that the outer half does nothing but parameter wrapping. For instance, void qapi_event_send_block_io_error(const char *device, IoOperationType operation, BlockErrorAction action, bool has_nospace, bool nospace, const char *reason, Error **errp) { QDict *qmp; Error *err = NULL; QMPEventFuncEmit emit; QmpOutputVisitor *qov; Visitor *v; QObject *obj; _obj_BLOCK_IO_ERROR_arg param = { (char *)device, operation, action, has_nospace, nospace, (char *)reason }; [do stuff...] } becomes static inline void do_event_send_block_io_error(_obj_BLOCK_IO_ERROR_arg param, Error **errp) { QDict *qmp; Error *err = NULL; QMPEventFuncEmit emit; QmpOutputVisitor *qov; Visitor *v; QObject *obj; [do stuff...] } void qapi_event_send_block_io_error(const char *device, IoOperationType operation, BlockErrorAction action, bool has_nospace, bool nospace, const char *reason, Error **errp) { do_event_send_block_io_error((_obj_BLOCK_IO_ERROR_arg){ (char *)device, operation, action, has_nospace, nospace, (char *)reason }, errp); }; } Feel free not to do that now, but mark the spot with a comment then. Since it's technically wrong, we could even mark it FIXME. >> >>> + assert not typ.variants >>> + ret = mcgen(''' >>> + %(c_name)s qapi = { >>> +''', >>> + c_name=typ.c_name()) >>> + sep = ' ' >>> + for memb in typ.members: >>> + ret += sep >>> + sep = ', ' >>> + if memb.optional: >>> + ret += 'has_' + c_name(memb.name) + sep >>> + if memb.type.name == 'str': >>> + # Cast away const added in gen_params() >>> + ret += '(char *)' >> >> Why is that useful? > > The compiler complains if you try to initialize a 'char *' member of a > QAPI C struct with a 'const char *' parameter. It's no different than > the fact that the gen_visit_members() call we are getting rid of also > has to cast away const. I see. Const never fails to annoy. [...]