Re: [Qemu-devel] [PATCH v6 03/12] qapi: Lazy creation of array types

[Eric Blake]

On 10/02/2015 02:06 AM, Markus Armbruster wrote:
> Woot!
> 
> Eric Blake <address@hidden> writes:
> 
>> Commit ac88219a had several TODO markers about whether we needed
>> to automatically create the corresponding array type alongside
>> any other type.  It turns out that most of the time, we don't!
>>
>> As part of lazy creation of array types, this patch now assigns
>> an 'info' to array types at their point of first instantiation,
>> rather than leaving it None.
> 
> I guess our general for info should be:

s/general/general description/ ?

> 
> For explicitly defined entities, info points to the (explicit)
> definition.  For implicitly defined entities, it points to a place that
> triggers implicit definition.  For some kinds of entities, multiple
> places may exist, and info points to one of them.
> 

Right now, we don't document any of the fields, but I can certainly add
that comment.


> 
> Here, you're talking about the effect on generated code.  I'd put the
> specific case of introspection last, in a paragraph of its own.
> 

Improving the commit message is easier than redoing bad code :)

>> +++ b/qapi-schema.json
>> @@ -3396,6 +3396,16 @@
>>              'features': 'int' } }
>>
>>  ##
>> +# @Dummy
>> +#
>> +# Not used by QMP; hack to let us use X86CPUFeatureWordInfoList internally
>> +#
>> +# Since 2.5
>> +##
>> +{ 'struct': 'Dummy', 'data': { 'unused': ['X86CPUFeatureWordInfo'] } }
> 
> DummyToForceArrays?

Sure.

> 
>> +
>> +
>> +##
>>  # @RxState:
>>  #
>>  # Packets receiving state
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 8123ab3..15640b6 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -1143,7 +1143,7 @@ class QAPISchema(object):
>>      def _def_builtin_type(self, name, json_type, c_type, c_null):
>>          self._def_entity(QAPISchemaBuiltinType(name, json_type,
>>                                                 c_type, c_null))
>> -        self._make_array_type(name)     # TODO really needed?
>> +        self._make_array_type(name, None)
> 
> Should we keep a TODO here?

Sure, with better text explaining the guard issue on types shared in
multiple qapi-types.h files.


>>          typ = self._make_implicit_object_type(typ, 'wrapper',
>> -                                              [self._make_member('data', 
>> typ)])
>> +                                              [self._make_member('data', 
>> typ,
>> +                                                                 info)])
> 
> Consider a hanging indent here:
> 
>         typ = self._make_implicit_object_type(
>                     typ, 'wrapper', [self._make_member('data', typ, info)])


Okay.  pep8 and pylint like it (I'm not used to doing it, but it does
look better, and emacs didn't fight me too hard).

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

signature.asc
Description: OpenPGP digital signature