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

[Markus Armbruster]

Eric Blake  writes:

> On 02/16/2016 10:03 AM, Markus Armbruster wrote:
>> Eric Blake  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

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

[Eric Blake]

On 02/16/2016 10:03 AM, Markus Armbruster wrote:
> Eric Blake  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 += ...

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

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



signature.asc
Description: OpenPGP digital signature

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

[Markus Armbruster]

Eric Blake  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).
>
> Signed-off-by: Eric Blake 
>
> ---
> v10: new patch
> ---
>  scripts/qapi-types.py | 23 ---
>  1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index 83f230a..2f23432 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -2,7 +2,7 @@
>  # QAPI types generator
>  #
>  # Copyright IBM, Corp. 2011
> -# Copyright (c) 2013-2015 Red Hat Inc.
> +# Copyright (c) 2013-2016 Red Hat Inc.
>  #
>  # Authors:
>  #  Anthony Liguori 
> @@ -14,6 +14,11 @@
>  from qapi import *
>
>
> +# variants must be emitted before their container; track what has already
> +# been output
> +objects_seen = set()
> +
> +
>  def gen_fwd_object_or_array(name):
>  return mcgen('''
>
> @@ -49,11 +54,23 @@ def gen_struct_fields(members):
>
>
>  def gen_object(name, base, members, variants):
> -ret = mcgen('''
> +if name in objects_seen:
> +return ''
> +objects_seen.add(name)
> +
> +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()):

> +
> +ret += mcgen('''
>
>  struct %(c_name)s {
>  ''',
> -c_name=c_name(name))
> + c_name=c_name(name))
>
>  if base:
>  ret += mcgen('''