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

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>>  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 <address@hidden>

Thanks!