From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41199) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHega-0006AB-Lz for qemu-devel@nongnu.org; Tue, 21 Jul 2015 17:02:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZHegW-0006VY-EI for qemu-devel@nongnu.org; Tue, 21 Jul 2015 17:02:48 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50725) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHegW-0006VU-5Z for qemu-devel@nongnu.org; Tue, 21 Jul 2015 17:02:44 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-23-git-send-email-armbru@redhat.com> From: Eric Blake Message-ID: <55AEB36D.1040507@redhat.com> Date: Tue, 21 Jul 2015 15:02:37 -0600 MIME-Version: 1.0 In-Reply-To: <1435782155-31412-23-git-send-email-armbru@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mI6XsQ7UdHghLWRoPoK2KEvrNXRGgCKqp" Subject: Re: [Qemu-devel] [PATCH RFC v2 22/47] qapi: QAPISchema code generation helper methods List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , qemu-devel@nongnu.org Cc: kwolf@redhat.com, berto@igalia.com, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --mI6XsQ7UdHghLWRoPoK2KEvrNXRGgCKqp Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/01/2015 02:22 PM, Markus Armbruster wrote: > New methods c_name(), c_type(), c_null(), json_type(), > alternate_qtype(). >=20 > Signed-off-by: Markus Armbruster > --- > scripts/qapi.py | 72 +++++++++++++++++++++++++++++++++++++++++++++++++= ++------ > 1 file changed, 65 insertions(+), 7 deletions(-) Still unused in the rest of the code, so no change to generated code yet; and thanks for separating this apart from the creation of the new object hierarchy. > class QAPISchemaType(QAPISchemaEntity): > - pass > + def c_type(self, is_param=3DFalse): > + return c_name(self.name) + pointer_suffix At first, I thought is_param was unused; but reviewing the rest of the patch shows that subclasses are overriding it, so declaring it here is necessary for polymorphic calls to supply the argument and have it properly acted on. > + def c_null(self): > + return 'NULL' > + def json_type(self): > + return None > + def alternate_qtype(self): > + json2qtype =3D { > + 'string': 'QTYPE_QSTRING', > + 'number': 'QTYPE_QFLOAT', > + 'int': 'QTYPE_QINT', > + 'boolean': 'QTYPE_QBOOL', > + 'object': 'QTYPE_QDICT' > + } Are there any style rules on whether to include or omit a trailing comma?= > + return json2qtype.get(self.json_type()) > =20 > class QAPISchemaBuiltinType(QAPISchemaType): > - def __init__(self, name): > + def __init__(self, name, json_type, c_type, c_null): > QAPISchemaType.__init__(self, name, None) > + assert not c_type or isinstance(c_type, str) > + assert json_type in ('string', 'number', 'int', 'boolean', 'nu= ll', > + 'value') > + self.json_type_name =3D json_type > + self.c_type_name =3D c_type > + self.c_null_val =3D c_null > + def c_name(self): > + return self.name At first, I thought this was redundant with QAPISchemaEntity.c_name(), until I realized that you are intentionally overriding things here to NOT call global c_name() (so that 'int' does not get munged the 'q_int' in the generated C code). Nice, once I figured it out. > + def c_type(self, is_param=3DFalse): > + if is_param and self.name =3D=3D 'str': > + return 'const ' + self.c_type_name > + return self.c_type_name > + def c_null(self): > + return self.c_null_val > + def json_type(self): > + return self.json_type_name > =20 > class QAPISchemaEnumType(QAPISchemaType): > def __init__(self, name, info, values): > @@ -779,6 +811,12 @@ class QAPISchemaEnumType(QAPISchemaType): > for v in values: > assert isinstance(v, str) > self.values =3D values > + def c_type(self, is_param=3DFalse): > + return c_name(self.name) Probably showing my unfamiliarity with python polymorphism/overriding rules - is it necessary to declare an otherwise-unused optional parameter here if we are overriding a base method where the parameter has a sane default for our needs? That is, would c_type(self) here properly override c_type(self, is_param=3DFalse) in the parent class? > + def c_null(self): > + return c_enum_const(self.name, self.values[0]) > + def json_type(self): > + return 'string' > =20 > class QAPISchemaArrayType(QAPISchemaType): > def __init__(self, name, info, element_type): > @@ -822,6 +860,14 @@ class QAPISchemaObjectType(QAPISchemaType): > if self.variants: > self.variants.check(schema, members, seen) > self.members =3D members > + def c_name(self): > + assert self.info > + return QAPISchemaType.c_name(self) > + def c_type(self, is_param=3DFalse): > + assert self.info > + return QAPISchemaType.c_type(self) So, to make sure I understand correctly, implicit types (those generated from anonymous dictionaries in a command or event) have info=3DNone, and the assertions here are making sure we never try to learn the c_name or c_type of those implicit types in generated code, but rather use them solely in generated code that just enumerates the members (C struct declarations, parameter list to functions that implement commands). > + def json_type(self): > + return 'object' > =20 > class QAPISchemaObjectTypeMember(object): > def __init__(self, name, typ, optional): > @@ -948,15 +994,27 @@ class QAPISchema(object): > def lookup_type(self, name): > return self.lookup_entity(name, QAPISchemaType) > =20 > - def _def_builtin_type(self, name): > - self._def_entity(QAPISchemaBuiltinType(name)) > + def _def_builtin_type(self, name, json_type, c_type, c_null): > + self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type= , c_null)) > if name !=3D '**': > self._make_array_type(name) # TODO really needed? > =20 > def _def_predefineds(self): > - for t in ['str', 'number', 'int', 'int8', 'int16', 'int32', 'i= nt64', > - 'uint8', 'uint16', 'uint32', 'uint64', 'size', 'bool= ', '**']: > - self._def_builtin_type(t) > + for t in [('str', 'string', 'char' + pointer_suffix, 'NULL= '), > + ('number', 'number', 'double', '0'), Do you want '0.0' as the default here (yes, promoting an int 0 to double does the right thing, but I like making it obvious that I know I'm using floating point). > + ('int', 'int', 'int64_t', '0'), > + ('int8', 'int', 'int8_t', '0'), > + ('int16', 'int', 'int16_t', '0'), > + ('int32', 'int', 'int32_t', '0'), > + ('int64', 'int', 'int64_t', '0'), > + ('uint8', 'int', 'uint8_t', '0'), > + ('uint16', 'int', 'uint16_t', '0'), > + ('uint32', 'int', 'uint32_t', '0'), > + ('uint64', 'int', 'uint64_t', '0'), > + ('size', 'int', 'uint64_t', '0'), > + ('bool', 'boolean', 'bool', 'false'), > + ('**', 'value', None, None)]: > + self._def_builtin_type(*t) > =20 > def _make_implicit_enum_type(self, name, values): > name =3D name + 'Kind' >=20 Tweaks you make (if any) based on the above comments should be minor, so I'm fine with adding: Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --mI6XsQ7UdHghLWRoPoK2KEvrNXRGgCKqp 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/ iQEcBAEBCAAGBQJVrrNtAAoJEKeha0olJ0NqbRAH/1OLoSDfOeFdu9/NidaECOx/ QG3NH3vb5yqpQbkwPByG2A8yzpK83ZdXNeyYszDeIZEV93a/6MWEkyiktJReE+6d zis7jWWWL0pm6NebPFM6+qCX1lwSMNGvkjMSfNNpqDuM2HIvU5DiuacGAH9WsI+T k1VH75HwbPSXLTlzcMjI324RDQ1PWixtKEh6el+MGlglGh2VR1e0GXXzFW3mvpi8 4oqypOIPUDdBpdhQJoC8/aoUU7sQVzXCZuP5pdxeXQFEzvZSAWUJbtf+SPCbPW7b YKz/hK04CLolKrP+Xd8UHbaTXuh2Q8c6pgEqG5xwfom5aiwEWcnuWKF1JnS2a/4= =qbzj -----END PGP SIGNATURE----- --mI6XsQ7UdHghLWRoPoK2KEvrNXRGgCKqp--