Re: [Qemu-devel] [PATCH RFC v2 22/47] qapi: QAPISchema code generation helper methods
Eric Blake writes: > On 07/27/2015 03:54 AM, Markus Armbruster wrote: >> 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(-) >>> >>> I just noticed: >>> @@ -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) +def c_null(self): +return c_enum_const(self.name, self.values[0]) >>> >>> What does this return for an empty enum, as in { 'enum':'Empty', >>> 'data':[] }? >> >> I suspect self.values will be [] then, and self.values[0] will bomb. >> >> Possible fixes: >> >> * Outlaw empty enums >> >> * Add the implicit MAX member to self.values[] (other code may have to >> skip it) >> >> * Catch the special case here, and return the implicit MAX member. > > I'm leaning toward the third option here. Me too now, because it's easy: return c_enum_const(self.name, (self.values + [ 'MAX' ])[0]) >>> Our testsuite proves we can do that, even if our normal >>> .json code doesn't use it. >> >> tests/qapi-schema/enum-empty.json:{ 'enum': 'MyEnum', 'data': [ ] } > > As I've mentioned elsewhere, most of our tests/qapi-schema/*.json merely > cover whether the parser is okay with the input, while > tests/qapi-schema/qapi-schema-test.json is the one that also tests that > the generated C code works; so sounds like that test should be enhanced > to cover some of these corner cases we have been considering in this > series (empty enum, command with no arguments and no returns, and so forth). Yes, that would be nice, but I feel I have to leave it for another day.
Re: [Qemu-devel] [PATCH RFC v2 22/47] qapi: QAPISchema code generation helper methods
On 07/27/2015 03:54 AM, Markus Armbruster wrote: > 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(-) >>> >> >> I just noticed: >> >>> @@ -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) >>> +def c_null(self): >>> +return c_enum_const(self.name, self.values[0]) >> >> What does this return for an empty enum, as in { 'enum':'Empty', >> 'data':[] }? > > I suspect self.values will be [] then, and self.values[0] will bomb. > > Possible fixes: > > * Outlaw empty enums > > * Add the implicit MAX member to self.values[] (other code may have to > skip it) > > * Catch the special case here, and return the implicit MAX member. I'm leaning toward the third option here. > >> Our testsuite proves we can do that, even if our normal >> .json code doesn't use it. > > tests/qapi-schema/enum-empty.json:{ 'enum': 'MyEnum', 'data': [ ] } As I've mentioned elsewhere, most of our tests/qapi-schema/*.json merely cover whether the parser is okay with the input, while tests/qapi-schema/qapi-schema-test.json is the one that also tests that the generated C code works; so sounds like that test should be enhanced to cover some of these corner cases we have been considering in this series (empty enum, command with no arguments and no returns, and so forth). -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH RFC v2 22/47] qapi: QAPISchema code generation helper methods
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(-) >> > > I just noticed: > >> @@ -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) >> +def c_null(self): >> +return c_enum_const(self.name, self.values[0]) > > What does this return for an empty enum, as in { 'enum':'Empty', > 'data':[] }? I suspect self.values will be [] then, and self.values[0] will bomb. Possible fixes: * Outlaw empty enums * Add the implicit MAX member to self.values[] (other code may have to skip it) * Catch the special case here, and return the implicit MAX member. > Our testsuite proves we can do that, even if our normal > .json code doesn't use it. tests/qapi-schema/enum-empty.json:{ 'enum': 'MyEnum', 'data': [ ] }
Re: [Qemu-devel] [PATCH RFC v2 22/47] qapi: QAPISchema code generation helper methods
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): >> -sel
Re: [Qemu-devel] [PATCH RFC v2 22/47] qapi: QAPISchema code generation helper methods
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(-) > I just noticed: > @@ -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) > +def c_null(self): > +return c_enum_const(self.name, self.values[0]) What does this return for an empty enum, as in { 'enum':'Empty', 'data':[] }? Our testsuite proves we can do that, even if our normal .json code doesn't use it. -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH RFC v2 22/47] qapi: QAPISchema code generation helper methods
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', > -
[Qemu-devel] [PATCH RFC v2 22/47] qapi: QAPISchema code generation helper methods
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(-) diff --git a/scripts/qapi.py b/scripts/qapi.py index a80892e..b5fa1be 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -763,15 +763,47 @@ class QAPISchemaEntity(object): assert isinstance(name, str) self.name = name self.info = info +def c_name(self): +return c_name(self.name) def check(self, schema): pass class QAPISchemaType(QAPISchemaEntity): -pass +def c_type(self, is_param=False): +return c_name(self.name) + pointer_suffix +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' +} +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 +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) +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) +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'), + ('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' -- 1.9.3