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

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On 10/02/2015 02:06 AM, Markus Armbruster wrote:
>> Woot!
>> 
>> Eric Blake <eblake@redhat.com> 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/ ?

General idea.

Looks like my typing engine doesn't run on all cylinders on Fridays...

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

I'm still getting used to hanging indents myself.  When in Rome...