Re: [Qemu-devel] [PATCH RFC v2 22/47] qapi: QAPISchema code generation helper methods

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6706 bytes --]

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 <armbru@redhat.com>
> ---
>  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 <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]