| Message ID | 1456262075-3311-3-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@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 <eblake@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().
On 02/24/2016 05:28 AM, Markus Armbruster wrote: > Eric Blake <eblake@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 <eblake@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.
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 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) - - struct_fields_seen.add(name) - 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) { 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) ret += mcgen(''' @@ -305,7 +284,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): 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) defn = gen_visit_list(name, element_type) if isinstance(element_type, QAPISchemaBuiltinType): self._btin += decl
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 <eblake@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(-)