Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com> > > --- > 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.