Re: [Qemu-devel] [PATCH v10 09/13] qapi: Emit structs used as variants in topological order

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 02/16/2016 10:03 AM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>> 
>>> Right now, we emit the branches of union types as a boxed pointer,
>>> and it suffices to have a forward declaration of the type.  However,
>>> a future patch will swap things to directly use the branch type,
>>> instead of hiding it behind a pointer.  For this to work, the
>>> compiler needs the full definition of the type, not just a forward
>>> declaration, prior to the union that is including the branch type.
>>> This patch just adds topological sorting to hoist all types
>>> mentioned in a branch of a union to be fully declared before the
>>> union itself.  The sort is always possible, because we do not
>>> allow circular union types that include themselves as a direct
>>> branch (it is, however, still possible to include a branch type
>>> that itself has a pointer to the union, for a type that can
>>> indirectly recursively nest itself - that remains safe, because
>>> that the member of the branch type will remain a pointer, and the
>>> QMP representation of such a type adds another {} for each recurring
>>> layer of the union type).
>>>
>
>>> +    ret = ''
>>> +    if variants:
>>> +        for v in variants.variants:
>>> +            if isinstance(v.type, QAPISchemaObjectType) and \
>>> +               not v.type.is_implicit():
>>> +                ret += gen_object(v.type.name, v.type.base,
>>> +                                  v.type.local_members, v.type.variants)
>> 
>> PEP 8:
>> 
>>     The preferred way of wrapping long lines is by using Python's
>>     implied line continuation inside parentheses, brackets and
>>     braces. Long lines can be broken over multiple lines by wrapping
>>     expressions in parentheses. These should be used in preference to
>>     using a backslash for line continuation.
>> 
>> In this case:
>> 
>>                if (isinstance(v.type, QAPISchemaObjectType) and
>>                    not v.type.is_implicit()):
>
> pep8 silently accepted my version, but complains about yours:
>
> scripts/qapi-types.py:65:5: E129 visually indented line with same indent
> as next logical line
>
> So the compromise for both of us is added indentation:
>
>         if (isinstance(v.type, QAPISchemaObjectType) and
>                 not v.type.is_implicit()):
>             ret += ...

Sold.

>
> Or, I could revisit my earlier proposal of:
>
> v.type.is_implicit(QAPISchemaObjectType)
>
> of giving .is_implicit() an optional parameter; if absent, all types are
> considered, but if present, the predicate is True only if the type of
> the object being queried matches the parameter type name.
>
> Here's the last time we discussed the tradeoffs of the shorter form:
> https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg02272.html