| Message ID | 1455582057-27565-10-git-send-email-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
Eric Blake <eblake@redhat.com> 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 <eblake@redhat.com> > > --- > 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 <aliguori@us.ibm.com> > @@ -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('''
On 02/16/2016 10:03 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> 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> writes: > On 02/16/2016 10:03 AM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> 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
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 <aliguori@us.ibm.com> @@ -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) + + ret += mcgen(''' struct %(c_name)s { ''', - c_name=c_name(name)) + c_name=c_name(name)) if base: ret += mcgen('''
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 <eblake@redhat.com> --- v10: new patch --- scripts/qapi-types.py | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)