From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46472) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJepy-0002x5-H8 for qemu-devel@nongnu.org; Mon, 27 Jul 2015 05:36:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZJept-0007d9-TK for qemu-devel@nongnu.org; Mon, 27 Jul 2015 05:36:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59631) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZJept-0007d5-Lz for qemu-devel@nongnu.org; Mon, 27 Jul 2015 05:36:41 -0400 From: Markus Armbruster References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-23-git-send-email-armbru@redhat.com> <55AEB36D.1040507@redhat.com> Date: Mon, 27 Jul 2015 11:36:38 +0200 In-Reply-To: <55AEB36D.1040507@redhat.com> (Eric Blake's message of "Tue, 21 Jul 2015 15:02:37 -0600") Message-ID: <871tfuq6vt.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: 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:22 PM, Markus Armbruster wrote: >> New methods c_name(), c_type(), c_null(), json_type(), >> alternate_qtype(). >> >> 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=False): >> + 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. Yup. >> + def c_null(self): >> + return 'NULL' >> + def json_type(self): >> + return None >> + def alternate_qtype(self): >> + json2qtype = { >> + '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? PEP8 appears to be silent on it. >> + return json2qtype.get(self.json_type()) >> >> 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', 'null', >> + 'value') >> + self.json_type_name = json_type >> + self.c_type_name = c_type >> + self.c_null_val = 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. I'll consider adding comments to help future readers figure it out. >> + def c_type(self, is_param=False): >> + if is_param and self.name == '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 >> >> 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 = values >> + def c_type(self, is_param=False): >> + 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=False) in the parent class? I don't know. But the way I wrote it, we don't need to know :) >> + def c_null(self): >> + return c_enum_const(self.name, self.values[0]) >> + def json_type(self): >> + return 'string' >> >> 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 = members >> + def c_name(self): >> + assert self.info >> + return QAPISchemaType.c_name(self) >> + def c_type(self, is_param=False): >> + 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=None, Correct. > 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). Correct again. >> + def json_type(self): >> + return 'object' >> >> 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) >> >> - 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 != '**': >> self._make_array_type(name) # TODO really needed? >> >> def _def_predefineds(self): >> - for t in ['str', 'number', 'int', 'int8', 'int16', 'int32', 'int64', >> - '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). The '0' here is used as initializer, i.e. we generate things like double d = 0; Changing it to '0.0' will instead generate double d = 0.0; No strong preference on my part. >> + ('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) >> >> def _make_implicit_enum_type(self, name, values): >> name = name + 'Kind' >> > > Tweaks you make (if any) based on the above comments should be minor, so > I'm fine with adding: > > Reviewed-by: Eric Blake Thanks!