Eric Blake <ebl...@redhat.com> writes:
> Dan Berrange reported a case where he needs to work with a
> QCryptoBlockOptions union type using the OptsVisitor, but only
> visit one of the branches of that type (the discriminator is not
> visited directly, but learned externally). When things were
> boxed, it was easy: just visit the variant directly, which took
> care of both allocating the variant and visiting its fields. But
> now that things are unboxed, we need a way to visit the fields
> without allocation, done by exposing visit_type_FOO_fields() to
> the user. Of course, this should only be done for objects, not
> lists, so we need another flag to gen_visit_decl().
>
> Since the function is now public, we no longer need to preserve
> topological ordering via struct_fields_seen.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> Minor conflicts with pending series "qapi implicit types"; I can
> rebase whichever series gets reviewed second.
> ---
> scripts/qapi-visit.py | 47 +++++++++++++----------------------------------
> 1 file changed, 13 insertions(+), 34 deletions(-)
>
Let me review how this thing works for an object type FOO before your
patch.
gen_visit_object() generates visit_type_FOO() with external linkage, and
gen_visit_struct_fields() generates visit_type_FOO_fields() with
internal linkage.
gen_visit_decl() generates a declaration of visit_type_FOO(), and
gen_visit_fields_decl() generates one for visit_type_FOO_fields() unless
it's already in scope.
visit_type_FOO_fields() is always called by visit_type_FOO(), and
sometimes called elsewhere.
We generate visit_type_FOO_fields() right before visit_type_FOO(), so
it's in scope there. Anything that generates uses elsewhere must call
gen_visit_fields_decl().
Your patch generates visit_type_FOO_fields() declarations into the
header, which renders the "if already in scope" logic useless, along
with the need to call gen_visit_fields_decl() before generating
visit_type_FOO_fields() uses outside visit_type_FOO().
> diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
> index 2308268..35efe7c 100644
> --- a/scripts/qapi-visit.py
> +++ b/scripts/qapi-visit.py
> @@ -15,52 +15,33 @@
> from qapi import *
> import re
>
> -# visit_type_FOO_fields() is always emitted; track if a forward declaration
> -# or implementation has already been output.
> -struct_fields_seen = set()
>
> -
> -def gen_visit_decl(name, scalar=False):
> +def gen_visit_decl(name, scalar=False, list=False):
> + ret = ''
> c_type = c_name(name) + ' *'
> if not scalar:
> + if not list:
> + ret += mcgen('''
> +void visit_type_%(c_name)s_fields(Visitor *v, %(c_type)sobj, Error **errp);
> +''',
> + c_name=c_name(name), c_type=c_type)
> c_type += '*'
> - return mcgen('''
> + ret += mcgen('''
> void visit_type_%(c_name)s(Visitor *v, const char *name, %(c_type)sobj,
> Error **errp);
> ''',
> c_name=c_name(name), c_type=c_type)
> -
> -
> -def gen_visit_fields_decl(typ):
> - if typ.name in struct_fields_seen:
> - return ''
> - struct_fields_seen.add(typ.name)
> - return mcgen('''
> -
> -static void visit_type_%(c_type)s_fields(Visitor *v, %(c_type)s *obj, Error
> **errp);
> -''',
> - c_type=typ.c_name())
> + return ret
This folds gen_visit_fields_decl() into gen_visit_decl() with the
necessary changes: drop the "is already in scope" conditional, switch to
external linkage.
Since gen_visit_decl() is called for non-object types as well, it gets a
new optional parameter to suppress generation of
visit_type_FOO_fields(). The parameter is named @list, which makes no
sense to me. See more below.
I don't like do-everything functions with suppressor flags much. Can we
structure this as a set of do-one-thing functions? Possibly with
convenience functions collecting common sets.
> def gen_visit_struct_fields(name, base, members, variants):
> - ret = ''
> + ret = mcgen('''
>
> - if base:
> - ret += gen_visit_fields_decl(base)
> - if variants:
> - for var in variants.variants:
> - # Ugly special case for simple union TODO get rid of it
> - if not var.simple_union_type():
> - ret += gen_visit_fields_decl(var.type)
Drop gen_visit_fields_decl() calls. Good.
> -
> - struct_fields_seen.add(name)
Drop code supporting "if already in scope" conditionals. Good.
> - ret += mcgen('''
> -
> -static void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error
> **errp)
> +void visit_type_%(c_name)s_fields(Visitor *v, %(c_name)s *obj, Error **errp)
Change linkage. Good.
> {
> Error *err = NULL;
>
> ''',
> - c_name=c_name(name))
> + c_name=c_name(name))
>
> if base:
> ret += mcgen('''
> @@ -173,8 +154,6 @@ def gen_visit_alternate(name, variants):
> for var in variants.variants:
> if var.type.alternate_qtype() == 'QTYPE_QINT':
> promote_int = 'false'
> - if isinstance(var.type, QAPISchemaObjectType):
> - ret += gen_visit_fields_decl(var.type)
Drop gen_visit_fields_decl() calls. Good.
>
> ret += mcgen('''
>
> @@ -305,7 +284,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor):
def visit_enum_type(self, name, info, values, prefix):
# Special case for our lone builtin enum type
# TODO use something cleaner than existence of info
if not info:
self._btin += gen_visit_decl(name, scalar=True)
No change, because scalar=True effectively implies list=True.
if do_builtins:
self.defn += gen_visit_enum(name)
else:
self.decl += gen_visit_decl(name, scalar=True)
Likewise.
> self.defn += gen_visit_enum(name)
>
> def visit_array_type(self, name, info, element_type):
> - decl = gen_visit_decl(name)
> + decl = gen_visit_decl(name, list=True)
No change, because list=True suppresses it.
> defn = gen_visit_list(name, element_type)
> if isinstance(element_type, QAPISchemaBuiltinType):
> self._btin += decl
if do_builtins:
self.defn += defn
else:
self.decl += decl
self.defn += defn
def visit_object_type(self, name, info, base, members, variants):
self.decl += gen_visit_decl(name)
Here, we now generate an additional visit_type_FOO_fields() declaration.
self.defn += gen_visit_object(name, base, members, variants)
def visit_alternate_type(self, name, info, variants):
self.decl += gen_visit_decl(name)
Bug: here too.
$ grep _BlockdevRef_fields q*[ch]
qapi-visit.h:void visit_type_BlockdevRef_fields(Visitor *v, BlockdevRef
*obj, Error **errp);
self.defn += gen_visit_alternate(name, variants)
Your choice of an optional argument to keep things unchanged effectively
hid the places that changed. Failed to fool me today, but don't expect
to remain lucky that way :)
One more thing: I never liked the name _fields. C struct and union
types don't have fields, they have members. Likewiese, JSON objects.
We shouldn't gratitously invent terminology. We've done that quite a
bit around QMP (JSON object vs. C QDict, JSON array vs. QList, QFLoat is
really a double, ...). Cleaning that up completely is probably hopeless
by now. But we can rename visit_type_FOO_fields() to something more
sensible, say visit_type_FOO_members(), or even
visit_type_FOO_unboxed().
