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. > + 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? > + 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. > + 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? > + 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, 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' > > 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). > + ('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 -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org