[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 <armbru@redhat.com>
---
 scripts/qapi.py | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 65 insertions(+), 7 deletions(-)

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>

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(-)
> 

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> 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 <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.

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

Thanks!

Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com>
>> ---
>>  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': [ ] }

On 07/27/2015 03:54 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com>
>>> ---
>>>  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> writes:

> On 07/27/2015 03:54 AM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> 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 <armbru@redhat.com>
>>>> ---
>>>>  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.