From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38981) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zoydd-0004xO-As for qemu-devel@nongnu.org; Wed, 21 Oct 2015 15:01:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZoydY-0002s3-PR for qemu-devel@nongnu.org; Wed, 21 Oct 2015 15:01:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:56842) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZoydY-0002rz-BI for qemu-devel@nongnu.org; Wed, 21 Oct 2015 15:01:24 -0400 References: <1444968943-11254-1-git-send-email-eblake@redhat.com> <1444968943-11254-7-git-send-email-eblake@redhat.com> <87oafsdsy4.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <5627E0FE.60207@redhat.com> Date: Wed, 21 Oct 2015 13:01:18 -0600 MIME-Version: 1.0 In-Reply-To: <87oafsdsy4.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="WhwRpme9WTI03EqaOgjuMM70cowOO7hTb" Subject: Re: [Qemu-devel] [PATCH v9 06/17] qapi-visit: Remove redundant functions for flat union base List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --WhwRpme9WTI03EqaOgjuMM70cowOO7hTb Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/21/2015 11:36 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> The code for visiting the base class of a child struct created >> visit_type_Base_fields() which covers all fields of Base; while >> the code for visiting the base class of a flat union created >> visit_type_Union_fields() covering all fields of the base >> except the discriminator. But if the base class were to always >> visit all its fields, then we wouldn't need a separate visit of >> the discriminator for a union. Not only is consistently >> visiting all fields easier to understand, it lets us share code. >> >> Now that gen_visit_struct_fields() can potentially collect more >> than one function into 'ret', a regular expression searching for >> whether a label was used may hit a false positive within the >> body of the first function. But using a regex was overkill, >> since we can easily determine when we jumped to a label. >> >> Signed-off-by: Eric Blake >> >> +++ b/scripts/qapi-visit.py >> @@ -90,7 +90,7 @@ static void visit_type_%(c_name)s_fields(Visitor *v,= %(c_name)s **obj, Error **e >> >> ret +=3D gen_visit_fields(members, prefix=3D'(*obj)->') >> >> - if re.search('^ *goto out;', ret, re.MULTILINE): >> + if base or members: >=20 > What if we have an empty base and no members? Empty base is a > pathological case, admittedly. I'm going to filter the re.search cleanups into its own prereq patch. But yes, it will need to care for empty base and no members (hmm, I really ought to add positive tests to qapi-schema-test for an empty inherited struct, to make sure I'm getting it right - even if we don't want that patch in the final series). > Diff is confusing (not your fault). Let me compare code before and > after real slow. I also plan for v10 to include a diff of the generated code in the commit message, if that will help make the change more obvious. >=20 > =3D Before =3D >=20 > def gen_visit_union(name, base, variants): > ret =3D '' >=20 > 0. base is None if and only if the union is simple. >=20 > 1. If it's a flat union, generate its visit_type_NAME_fields(). This where NAME is the union name. > function visits the union's non-variant members *except* the > discriminator. Since a simple union has no non-variant members other > than the discriminator, generate it only for flat unions. >=20 > if base: > members =3D [m for m in base.members if m !=3D variants.tag_m= ember] > ret +=3D gen_visit_struct_fields(name, None, members) >=20 > 2. Generate the visit_type_implicit_FOO() we're going to need. >=20 > for var in variants.variants: > # Ugly special case for simple union TODO get rid of it > if not var.simple_union_type(): > ret +=3D gen_visit_implicit_struct(var.type) Could be made slightly simpler by generating these while we iterate over cases (but we'd have to be careful to generate into multiple variables, and then concat together at the end, since we can't generate one function in the body of the other). >=20 > 3. Generate its visit_type_NAME(). >=20 >=20 > 3.a. If it's a flat union, generate the call of > visit_type_NAME_fields(). Not necessary for simple unions, see 1. Again, important to note that this was visit_type_UNION_fields(). > 3.b. Generate visit of discriminator. >=20 >=20 > 3.c. Generate visit of the active variant. >=20 > =3D After =3D >=20 > def gen_visit_union(name, base, variants): > ret =3D '' >=20 > 0. base is None if and only if the union is simple. >=20 > 1. If it's a flat union, generate its visit_type_NAME_fields(). This > function visits the union's non-variant members *including* the > discriminator. However, we generate it only for flat unions. Simple > unions have no non-variant members other than the discriminator. >=20 > if base: > ret +=3D gen_visit_struct_fields(base.name, base.base, > base.local_members) Note that this creates visit_type_BASE_fields() (a different function). >=20 > 2. Generate the visit_type_implicit_FOO() we're going to need. >=20 > for var in variants.variants: > # Ugly special case for simple union TODO get rid of it > if not var.simple_union_type(): > ret +=3D gen_visit_implicit_struct(var.type) >=20 > 3. Generate its visit_type_NAME(). >=20 >=20 > 3.a. If it's a flat union, generate the call of > visit_type_NAME_fields(). Not done for simple unions, see 1. Again, now NAME is the base name rather than the union name. Subtle but important difference! >=20 > if base: > ret +=3D mcgen(''' > visit_type_%(c_name)s_fields(v, (%(c_name)s **)obj, &err); > ''', > c_name=3Dbase.c_name()) >=20 > 3.b. If it's a simple union, generate the visit of the sole non-variant= > member inline. >=20 >=20 > 3.a+b. Generate the error check for visit of non-variant members >=20 > ret +=3D gen_err_check(label=3D'out_obj') >=20 > 3.c. Generate visit of the active variant. >=20 >=20 > Okay, the change to gen_visit_union() looks sane. Yes, you got it all correct. >=20 > Can we go one step further and generate and use visit_type_NAME_fields(= ) > even for simple unions? Not easily. Remember, for flat unions, we are calling visit_type_BASE_fields, not visit_type_UNION_fields. There is no base for a simple union. What I _am_ planning for future patches is the following: Change QAPISchemaObjectType for simple unions and alternates to set =2Elocal_members to the one-element implicit discriminator (right now, it= is always an empty array, and we even assert that bool(.local_members) and bool(.variants) are mutually-exclusive in at least qapi-types.py visit_object_type()). Flat unions would keep .local_members as an empty array (there is no local member, just the inherited members from the base class, which includes the inherited discriminator). Then merge gen_visit_struct() and gen_visit_union() to look like: if base: # includes discriminator for flat union call visit_type_BASE_fields() for m in .local_members # includes discriminator for simple union call visit_type_MEMBER() if variants emit switch statement to visit each branch But if we want, that 'for m in .local_members' block can indeed be implemented via a call to visit_type_UNION_fields(), if that is any more efficient to implement. I guess it also boils down to deciding if visit_type_FOO_fields() should continue to be unconditional (either for all types, or at least for all types with non-empty .local_members). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --WhwRpme9WTI03EqaOgjuMM70cowOO7hTb Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWJ+D+AAoJEKeha0olJ0NquXUH/RKfzin7Og+JEw2cHcgOuoH9 zBrhWTrCj+6MzzvB3g28Le3WvLZDiNICc9yi5OXoIkfELq8EOro5IRtCXWd8FQ27 RBkkX1nx8nPGMCQLyCfNbnM7JhW6hx/No+MniZDS1dGzZHEhpTTkwSXWnPzbJPsT mI4dJDHcLT3E8GWCUlwrbFnJNWct1PR0oaRylBol/sA2BENnAbx8h5/HVxkeEKq4 /3LfAs6Bq36jFD8rT9hOUd1+Vu0rc93il5IcfeFEirzP06dD2jXujUNQj3puNEh7 eJZTaStbvqKbYhlIyrGWm7hGNoYUKUjDq32RZFaQg3DkFT/oGbSD6NTo0JsNC0c= =M5E7 -----END PGP SIGNATURE----- --WhwRpme9WTI03EqaOgjuMM70cowOO7hTb--