On 02/24/2016 05:28 AM, Markus Armbruster wrote: > 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.
Sounds like you want this in first; I'll respin a single series with
both sets of patches, with this at the front.
>> ---
>> 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().
Correct. I'll try and incorporate some of this text in the commit
message for better justification.
>> +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)
>
> 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.
Sure; gen_visit_decl() stays unchanged, and gen_visit_decl_fields()
would be new and called only for objects.
>
> def visit_alternate_type(self, name, info, variants):
> self.decl += gen_visit_decl(name)
>
> Bug: here too.
Indeed - the declaration doesn't hurt, but there is no implementation to
back up the declaration, so it's better if I fix things to not add the
declaration. v2 will fix it.
>
> $ 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().
visit_type_FOO_members() sounds slightly nicer to me. I'll use that
terminology in v2 to see how things look.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
