From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33955) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZIbfc-0000cr-Qe for qemu-devel@nongnu.org; Fri, 24 Jul 2015 08:01:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZIbfY-0000Xc-PL for qemu-devel@nongnu.org; Fri, 24 Jul 2015 08:01:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57293) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZIbfY-0000XQ-I5 for qemu-devel@nongnu.org; Fri, 24 Jul 2015 08:01:40 -0400 From: Markus Armbruster References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-8-git-send-email-armbru@redhat.com> <55AD7F18.1010205@redhat.com> Date: Fri, 24 Jul 2015 14:01:35 +0200 In-Reply-To: <55AD7F18.1010205@redhat.com> (Eric Blake's message of "Mon, 20 Jul 2015 17:07:04 -0600") Message-ID: <87380d4vds.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH RFC v2 07/47] qapi: Generate a nicer struct for flat unions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com Eric Blake writes: > On 07/01/2015 02:21 PM, Markus Armbruster wrote: >> The struct generated for a flat union is weird: the members of its >> base are at the end, except for the union tag, which is renamed to >> 'kind' and put at the beginning. >> > >> Change to put all base members at the beginning, unadulterated. Not >> only is this easier to understand, it also permits casting the flat >> union to its base, if that should become useful. >> >> We now generate: >> >> struct UserDefFlatUnion >> { > > It might be worth tweaking the generator to output a C comment either > here (at the start of the larger struct)... > >> char *string; >> EnumOne enum1; > > ...or here (at the end of the base struct) mentioning that > UserDefFlatUnion can be cast into its base struct UserDefUnionBase, to > make it easier when reading the generated code to trace back to that > "inheritance" relationship. Right now, there is nothing in the > generated UserDefFlatUnion that points you back to the qapi relationship > of a base class. But it's not a show-stopper if you don't like my > suggestion. I do like it. Perhaps something like struct UserDefFlatUnion { /* Members inherited from UserDefUnionBase: */ char *string; EnumOne enum1; /* Own members: */ ... }; >> /* union tag is EnumOne enum1 */ >> union { >> void *data; >> UserDefA *value1; >> UserDefB *value2; >> UserDefB *value3; >> }; >> }; > > Only impact in files generated for qemu was to struct BlockdevOptions, > in the files qapi-types.[ch], and I agree that it is an improvement. > Oddly enough, it seems none of the C code cared about the field being > renamed from 'kind' to 'driver' (I guess that's because a lot of our use > of the blockdev code is not directly through the generated C structs, > but through QObject dictionary queries). It surprised me, too. >> Signed-off-by: Markus Armbruster >> --- >> scripts/qapi-types.py | 32 ++++++++++++++++++-------------- >> scripts/qapi-visit.py | 7 +++++-- >> tests/test-qmp-input-visitor.c | 2 +- >> tests/test-qmp-output-visitor.c | 2 +- >> 4 files changed, 25 insertions(+), 18 deletions(-) >> > >> +''', >> + name=name) >> + if base: >> + base_fields = find_struct(base)['data'] >> + ret += generate_struct_fields(base_fields) > >> - if base: >> - assert discriminator >> - base_fields = find_struct(base)['data'].copy() >> - del base_fields[discriminator] >> - ret += generate_struct_fields(base_fields) > > I also like the fact that you no longer modify the base_fields array > (because you are no longer floating a single renamed element > out-of-order compared to the rest of the base), and therefore avoid the > need for a .copy(). > > Reviewed-by: Eric Blake Thanks!