[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 05/14] qapi: Emit implicit structs in generat
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v5 05/14] qapi: Emit implicit structs in generated C |
Date: |
Thu, 10 Mar 2016 15:25:58 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> We already have several places that want to visit all the members
> of an implicit object within a larger context (simple union variant,
> event with anonymous data, command with anonymous arguments struct);
> and will be adding another one soon (the ability to declare an
> anonymous base for a flat union). Having a C struct declared for
> these implicit types, along with a visit_type_FOO_members() helper
> function, will make for fewer special cases in our generator.
>
> We do not, however, need qapi_free_FOO() or visit_type_FOO()
> functions for implicit types, because they should not be used
> directly outside of the generated code. This is done by adding a
> conditional in visit_object_type() for both qapi-types.py and
> qapi-visit.py based on the object name. The comparison of
> "name.startswith('q_')" is a bit hacky (it's basically duplicating
> what .is_implicit() already uses), but beats changing the signature
> of the visit_object_type() callback to pass a new 'implicit' flag.
> The hack should be temporary: we are considering adding a future
> patch that consolidates the narrow visit_object_type() and
> visit_object_type_flat() [with different pieces already broken out]
> into a broader visit_object_type() [where the visitor can query the
> type object directly].
>
> Also, now that we WANT to output C code for implicits, we have to
> change the condition in the visit_needed() filter. We have to
> special-case 'q_empty' in that filter as something that does not
> get output: because it is a built-in type, it would be emitted in
> multiple files (the main qapi-types.h and in qga-qapi-types.h)
> causing compilation failure due to redefinition. But since it
> has no members, it's easier to just avoid an attempt to visit
> that particular type.
Not just easier. ':empty' is a genuinely internal thing so far (it
exists only to make qapi-introspect.py simpler), and generating C for it
would be weird.
> The patch relies on the changed naming of implicit types in the
> previous patch. It is a bit unfortunate that the generated struct
> names and visit_type_FOO_members() don't match normal naming
> conventions, but it's not too bad, since they will only be used in
> generated code.
>
> The generated code grows substantially in size: the implicit
> '-wrapper' types must be emitted in qapi-types.h before any union
> can include an unboxed member of that type. Arguably, the '-args'
> types could be emitted in a private header for just qapi-visit.c
> and qmp-marshal.c, rather than polluting qapi-types.h; but adding
> complexity to the generator to split the output location according
> to role doesn't seem worth the maintenance costs. Another idea
> for a later patch would be reworking error.h to not need to
> include all of qapi-types.h.
I got that in my tree. If it goes in before this patch, the last
sentence should be dropped.
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v5: rebase onto different naming scheme, document future improvements
> v4: new patch
> ---
> scripts/qapi.py | 2 --
> scripts/qapi-types.py | 14 ++++++++------
> scripts/qapi-visit.py | 20 ++++++++++----------
> 3 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index f6701f5..96fb216 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1000,7 +1000,6 @@ class QAPISchemaObjectType(QAPISchemaType):
> return self.name.startswith('q_')
>
> def c_name(self):
> - assert not self.is_implicit()
> return QAPISchemaType.c_name(self)
>
> def c_type(self):
> @@ -1008,7 +1007,6 @@ class QAPISchemaObjectType(QAPISchemaType):
> return c_name(self.name) + pointer_suffix
>
> def c_unboxed_type(self):
> - assert not self.is_implicit()
> return c_name(self.name)
>
> def json_type(self):
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index f194bea..107eabe 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -61,8 +61,7 @@ def gen_object(name, base, members, variants):
> ret = ''
> if variants:
> for v in variants.variants:
> - if (isinstance(v.type, QAPISchemaObjectType) and
> - not v.type.is_implicit()):
> + if isinstance(v.type, QAPISchemaObjectType):
> ret += gen_object(v.type.name, v.type.base,
> v.type.local_members, v.type.variants)
>
> @@ -197,9 +196,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> self._btin = None
>
> def visit_needed(self, entity):
> - # Visit everything except implicit objects
> - return not (entity.is_implicit() and
> - isinstance(entity, QAPISchemaObjectType))
> + # Visit everything except the special empty builtin
> + return entity.name != 'q_empty'
>
> def _gen_type_cleanup(self, name):
> self.decl += gen_type_cleanup_decl(name)
Before: we visit all types except for implicit object types in "def
before use" order. visit_needed() filters out implicit object types.
gen_object() sorts by recursing, but filters out non-object types and
implicit object types. Note the shared "filters out implicit object
types" part.
After: we visit all types escept for the empty type in "def before use"
order. visit_needed() filters out the empty type. gen_object filters
out non-object types. It lost its shared part. If we somehow recurse
into the empty type, we visit it. Oops.
Actually, we overcomplicated things. visit_needed() exists so we can
filter out different kinds of entities in one condition. Really
convenient in qapi-introspect.py.
But if you want to filter out just one kind (here: object types), like
we do here, it's simpler to filter in that kind's visit_ method (here:
visit_object_type(). More efficient, too, but that doesn't matter.
Since we recurse, we need to additionally filter there. An easy way to
do that would be to start with objects_seen set to
set(schema.the_empty_object_type) instead of set().
> @@ -233,7 +231,11 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor):
> self.decl += gen_object(name, base, members, variants)
> if base:
> self.decl += gen_upcast(name, base)
> - self._gen_type_cleanup(name)
> + # FIXME Worth changing the visitor signature, so we could
> + # directly use rather than repeat type.is_implicit()?
Nitpick: this is a TODO, because it's not about fixing something that's
broken.
> + if not name.startswith('q_'):
> + # implicit types won't be directly allocated/freed
> + self._gen_type_cleanup(name)
>
> def visit_alternate_type(self, name, info, variants):
> self._fwdecl += gen_fwd_object_or_array(name)
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index a712e9a..c486aaa 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -215,13 +215,11 @@ out:
>
>
> def gen_visit_object(name, base, members, variants):
> - ret = gen_visit_object_members(name, base, members, variants)
> -
See visit_object_type() below.
> # FIXME: if *obj is NULL on entry, and visit_start_struct() assigns to
> # *obj, but then visit_type_FOO_members() fails, we should clean up *obj
> # rather than leaving it non-NULL. As currently written, the caller must
> # call qapi_free_FOO() to avoid a memory leak of the partial FOO.
> - ret += mcgen('''
> + return mcgen('''
>
> void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_name)s **obj,
> Error **errp)
> {
> @@ -245,8 +243,6 @@ out:
> ''',
> c_name=c_name(name))
>
> - return ret
> -
>
> class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
> def __init__(self):
> @@ -269,9 +265,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
> self._btin = None
>
> def visit_needed(self, entity):
> - # Visit everything except implicit objects
> - return not (entity.is_implicit() and
> - isinstance(entity, QAPISchemaObjectType))
> + # Visit everything except the special empty builtin
> + return entity.name != 'q_empty'
As in qapi-types.py, visit_needed() isn't necessary even before the
patch.
> def visit_enum_type(self, name, info, values, prefix):
> # Special case for our lone builtin enum type
> @@ -297,8 +292,13 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
>
> def visit_object_type(self, name, info, base, members, variants):
> self.decl += gen_visit_members_decl(name)
> - self.decl += gen_visit_decl(name)
> - self.defn += gen_visit_object(name, base, members, variants)
> + self.defn += gen_visit_object_members(name, base, members, variants)
Moved from gen_visit_object(), the rest can be called conditionally:
> + # FIXME Worth changing the visitor signature, so we could
> + # directly use rather than repeat type.is_implicit()?
> + if not name.startswith('q_'):
> + # only explicit types need an allocating visit
> + self.decl += gen_visit_decl(name)
> + self.defn += gen_visit_object(name, base, members, variants)
>
> def visit_alternate_type(self, name, info, variants):
> self.decl += gen_visit_decl(name)
Okay.
- Re: [Qemu-devel] [PATCH v5 06/14] qapi-event: Slightly shrink generated code, (continued)
- [Qemu-devel] [PATCH v5 07/14] qapi: Utilize implicit struct visits, Eric Blake, 2016/03/09
- [Qemu-devel] [PATCH v5 12/14] qapi: Make BlockdevOptions doc example closer to reality, Eric Blake, 2016/03/09
- [Qemu-devel] [PATCH v5 09/14] qapi: Inline gen_visit_members() into lone caller, Eric Blake, 2016/03/09
- [Qemu-devel] [PATCH v5 05/14] qapi: Emit implicit structs in generated C, Eric Blake, 2016/03/09
- Re: [Qemu-devel] [PATCH v5 05/14] qapi: Emit implicit structs in generated C,
Markus Armbruster <=
- [Qemu-devel] [PATCH v5 04/14] qapi: Adjust names of implicit types, Eric Blake, 2016/03/09
[Qemu-devel] [PATCH v5 13/14] qapi: Allow anonymous base for flat union, Eric Blake, 2016/03/09
[Qemu-devel] [PATCH v5 14/14] qapi: Use anonymous bases in QMP flat unions, Eric Blake, 2016/03/09