From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55791) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWRgG-00037s-Px for qemu-devel@nongnu.org; Thu, 18 Feb 2016 11:43:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aWRgC-00038k-JJ for qemu-devel@nongnu.org; Thu, 18 Feb 2016 11:43:52 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46088) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWRgC-00038g-Ar for qemu-devel@nongnu.org; Thu, 18 Feb 2016 11:43:48 -0500 From: Markus Armbruster References: <1455778109-6278-1-git-send-email-eblake@redhat.com> <1455778109-6278-13-git-send-email-eblake@redhat.com> Date: Thu, 18 Feb 2016 17:43:45 +0100 In-Reply-To: <1455778109-6278-13-git-send-email-eblake@redhat.com> (Eric Blake's message of "Wed, 17 Feb 2016 23:48:26 -0700") Message-ID: <874md69dfi.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v11 12/15] qapi: Don't box struct branch of alternate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > There's no reason to do two malloc's for an alternate type visiting > a QAPI struct; let's just inline the struct directly as the C union > branch of the struct. > > Surprisingly, no clients were actually using the struct member prior > to this patch outside of the testsuite; an earlier patch in the series > added some testsuite coverage to make the effect of this patch more > obvious. > > In qapi.py, c_type() gains a new is_unboxed flag to control when we > are emitting a C struct unboxed within the context of an outer > struct (different from our other two modes of usage with no flags > for normal local variable declarations, and with is_param for adding > 'const' in a parameter list). I don't know if there is any more > pythonic way of collapsing the two flags into a single parameter, > as we never have a caller setting both flags at once. > > Ultimately, we want to also unbox branches for QAPI unions, but as > that touches a lot more client code, it is better as separate > patches. But since unions and alternates share gen_variants(), I > had to hack in a way to test if we are visiting an alternate type > for setting the is_unboxed flag: look for a non-object branch. > This works because alternates have at least two branches, with at > most one object branch, while unions have only object branches. > The hack will go away in a later patch. > > The generated code difference to qapi-types.h is relatively small: > > | struct BlockdevRef { > | QType type; > | union { /* union tag is @type */ > | void *data; > |- BlockdevOptions *definition; > |+ BlockdevOptions definition; > | char *reference; > | } u; > | }; > > meanwhile, in qapi-visit.h, we trade the previous visit_type_FOO(obj) Meanwhile > (which allocates a pointer, handles a layer of {} from the JSON stream, > and visits the fields) with an inline call to visit_type_FOO(NULL) (to > consume the {} without allocation) and a direct visit of the fields: I don't see a call to visit_type_FOO(NULL). Suggest not to abbreviate argument lists, it's mildly confusing. > > | switch ((*obj)->type) { > | case QTYPE_QDICT: > |- visit_type_BlockdevOptions(v, name, &(*obj)->u.definition, &err); > |+ visit_start_struct(v, name, NULL, 0, &err); > |+ if (err) { > |+ break; > |+ } > |+ visit_type_BlockdevOptions_fields(v, &(*obj)->u.definition, &err); > |+ error_propagate(errp, err); > |+ err = NULL; > |+ visit_end_struct(v, &err); > | break; > | case QTYPE_QSTRING: > | visit_type_str(v, name, &(*obj)->u.reference, &err); > > The visit of non-object fields is unchanged. > > Signed-off-by: Eric Blake Patch looks good.