| Message ID | 1526029534-35771-2-git-send-email-anton.nefedov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Anton Nefedov <anton.nefedov@virtuozzo.com> writes: > The patch makes possible to avoid introducing dummy empty types > for the flat union branches that have no data. > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > --- > scripts/qapi/common.py | 18 ++++++++++++------ > scripts/qapi/doc.py | 2 +- > scripts/qapi/types.py | 2 +- > scripts/qapi/visit.py | 12 +++++++----- > tests/qapi-schema/test-qapi.py | 2 +- > 5 files changed, 22 insertions(+), 14 deletions(-) Doesn't docs/devel/qapi-code-gen.txt need an update? > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py > index a032cec..ec5cf28 100644 > --- a/scripts/qapi/common.py > +++ b/scripts/qapi/common.py > @@ -721,6 +721,7 @@ def check_union(expr, info): > name = expr['union'] > base = expr.get('base') > discriminator = expr.get('discriminator') > + partial = expr.get('data-partial', False) Yes, it does. Discussing a proposed new QAPI language feature is much easier when the patch starts with a documentation update specifying the new feature. If you were a seasoned QAPI developer, my review would stop right here :) Since you're not, I'll try to make sense of it. > members = expr['data'] > > # Two types of unions, determined by discriminator. > @@ -783,7 +784,7 @@ def check_union(expr, info): > % (key, enum_define['enum'])) > > # If discriminator is user-defined, ensure all values are covered > - if enum_define: > + if enum_define and not partial: data-partial supresses the check for "union lists all discriminator values". Makes sense given your commit message. > for value in enum_define['data']: > if value not in members.keys(): > raise QAPISemError(info, "Union '%s' data missing '%s' branch" > @@ -909,7 +910,7 @@ def check_exprs(exprs): > elif 'union' in expr: > meta = 'union' > check_keys(expr_elem, 'union', ['data'], > - ['base', 'discriminator']) > + ['base', 'discriminator', 'data-partial']) This accepts key 'data-partial'. Missing: we also need to check its value, in check_keys(). > union_types[expr[meta]] = expr > elif 'alternate' in expr: > meta = 'alternate' > @@ -1035,7 +1036,7 @@ class QAPISchemaVisitor(object): > def visit_array_type(self, name, info, element_type): > pass > > - def visit_object_type(self, name, info, base, members, variants): > + def visit_object_type(self, name, info, base, members, variants, partial): As we'll see below, @partial is needed just for qapi/visit.py's gen_visit_object_members(). > pass > > def visit_object_type_flat(self, name, info, members, variants): > @@ -1192,7 +1193,8 @@ class QAPISchemaArrayType(QAPISchemaType): > > > class QAPISchemaObjectType(QAPISchemaType): > - def __init__(self, name, info, doc, base, local_members, variants): > + def __init__(self, name, info, doc, base, local_members, variants, > + partial = False): > # struct has local_members, optional base, and no variants > # flat union has base, variants, and no local_members > # simple union has local_members, variants, and no base > @@ -1209,6 +1211,7 @@ class QAPISchemaObjectType(QAPISchemaType): > self.local_members = local_members > self.variants = variants > self.members = None > + self.partial = partial > > def check(self, schema): > if self.members is False: # check for cycles > @@ -1269,7 +1272,8 @@ class QAPISchemaObjectType(QAPISchemaType): > > def visit(self, visitor): > visitor.visit_object_type(self.name, self.info, > - self.base, self.local_members, self.variants) > + self.base, self.local_members, self.variants, > + self.partial) > visitor.visit_object_type_flat(self.name, self.info, > self.members, self.variants) > > @@ -1636,6 +1640,7 @@ class QAPISchema(object): > name = expr['union'] > data = expr['data'] > base = expr.get('base') > + partial = expr.get('data-partial', False) > tag_name = expr.get('discriminator') > tag_member = None > if isinstance(base, dict): Flat union; @partial applies. base = (self._make_implicit_object_type( name, info, doc, 'base', self._make_members(base, info))) if tag_name: variants = [self._make_variant(key, value) for (key, value) in data.items()] members = [] Note: if @partial, then variants no longer cover all tag values. As we'll see below, this necessitates changes to some back ends. Our general strategy is to make the front end normalize away convenience features as much as possible. It looks quite possible here: add the implicit variants. We need a type for them, obviously. Perhaps we can use the internal 'q_empty'. else: Simple union; @partial is meaningless here. variants = [self._make_simple_variant(key, value, info) for (key, value) in data.items()] typ = self._make_implicit_enum_type(name, info, [v.name for v in variants]) tag_member = QAPISchemaObjectTypeMember('type', typ, False) members = [tag_member] self._def_entity( QAPISchemaObjectType(name, info, doc, base, members, QAPISchemaObjectTypeVariants(tag_name, tag_member, variants))) > @@ -1656,7 +1661,8 @@ class QAPISchema(object): > QAPISchemaObjectType(name, info, doc, base, members, > QAPISchemaObjectTypeVariants(tag_name, > tag_member, > - variants))) > + variants), > + partial)) > > def _def_alternate_type(self, expr, info, doc): > name = expr['alternate'] Okay, that's all for the front end. As far as I can tell, 'data-partial' the front end accept and ignores 'data-partial' with simple unions. It should reject it instead. > diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py > index 9b312b2..40dffc4 100644 > --- a/scripts/qapi/doc.py > +++ b/scripts/qapi/doc.py > @@ -211,7 +211,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor): > body=texi_entity(doc, 'Values', > member_func=texi_enum_value))) > > - def visit_object_type(self, name, info, base, members, variants): > + def visit_object_type(self, name, info, base, members, variants, partial): > doc = self.cur_doc > if base and base.is_implicit(): > base = None > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py > index 64d9c0f..e805509 100644 > --- a/scripts/qapi/types.py > +++ b/scripts/qapi/types.py > @@ -215,7 +215,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor): > self._genh.add(gen_array(name, element_type)) > self._gen_type_cleanup(name) > > - def visit_object_type(self, name, info, base, members, variants): > + def visit_object_type(self, name, info, base, members, variants, partial): > # Nothing to do for the special empty builtin > if name == 'q_empty': > return > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py > index 5d72d89..3ee64bb 100644 > --- a/scripts/qapi/visit.py > +++ b/scripts/qapi/visit.py > @@ -34,7 +34,7 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp); > c_name=c_name(name)) > > > -def gen_visit_object_members(name, base, members, variants): > +def gen_visit_object_members(name, base, members, variants, partial): > ret = mcgen(''' > > void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) > @@ -93,9 +93,10 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) > > ret += mcgen(''' > default: > - abort(); > + %(action)s > } > -''') > +''', > + action="break;" if partial else "abort();") No change when partial=False: the switch cases cover all valid discriminator values, and default aborts on invalid ones. When partial=True, the cases cover only the values listed in the union, and default does nothing. This isn't quite right. We should do nothing only for the values not listed in the union, and still abort for invalid ones. If the front end normalizes away the convenience feature, we should get this behavior without any changes here. > > # 'goto out' produced for base, for each member, and if variants were > # present > @@ -309,12 +310,13 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): > self._genh.add(gen_visit_decl(name)) > self._genc.add(gen_visit_list(name, element_type)) > > - def visit_object_type(self, name, info, base, members, variants): > + def visit_object_type(self, name, info, base, members, variants, partial): > # Nothing to do for the special empty builtin > if name == 'q_empty': > return > self._genh.add(gen_visit_members_decl(name)) > - self._genc.add(gen_visit_object_members(name, base, members, variants)) > + self._genc.add(gen_visit_object_members(name, base, members, variants, > + partial)) > # TODO Worth changing the visitor signature, so we could > # directly use rather than repeat type.is_implicit()? > if not name.startswith('q_'): > diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py > index c1a144b..95cd575 100644 > --- a/tests/qapi-schema/test-qapi.py > +++ b/tests/qapi-schema/test-qapi.py > @@ -28,7 +28,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): > if prefix: > print(' prefix %s' % prefix) > > - def visit_object_type(self, name, info, base, members, variants): > + def visit_object_type(self, name, info, base, members, variants, partial): > print('object %s' % name) > if base: > print(' base %s' % base.name) Okay, now let's take a step back and review the problem this patch solves, and the design space. The perfect patch would do that in the commit message, but we're not expecting perfection, only honest effort :) Some unions have no variant members for certain discriminator values. We currently have to use an empty type there, and we've accumulated several just for the purpose, which is annoying. QAPI language design alternatives: 1. Having unions cover all discriminator values explicitly is useful. All we need is a more convenient way to denote empty types. Eric proposed {}, as in 'foo: {}. 2. Having unions repeat all the discriminator values explicitly is not useful. All we need is replacing the code enforcing that by code defaulting missing ones to the empty type. 3. We can't decide, so we do both (this patch). Preferences?
On 05/14/2018 01:08 PM, Markus Armbruster wrote: > Anton Nefedov <anton.nefedov@virtuozzo.com> writes: > >> The patch makes possible to avoid introducing dummy empty types >> for the flat union branches that have no data. >> > > Some unions have no variant members for certain discriminator values. > We currently have to use an empty type there, and we've accumulated > several just for the purpose, which is annoying. > > QAPI language design alternatives: > > 1. Having unions cover all discriminator values explicitly is useful. > All we need is a more convenient way to denote empty types. Eric > proposed {}, as in 'foo: {}. And even posted a patch for it once: https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00311.html although it was late enough in that series with other churn in the meantime that it would need serious revisiting to apply today. > > 2. Having unions repeat all the discriminator values explicitly is not > useful. All we need is replacing the code enforcing that by code > defaulting missing ones to the empty type. > > 3. We can't decide, so we do both (this patch). > > Preferences? Here's some tradeoffs to consider: Allowing 'foo':{} makes your intent explicit - "I know this branch of the union exists, but it specifically adds no further members". But it's a lot of redundant typing - "I already had to type all the branch names when declaring the enum type, why do it again here?" Allowing an easy way to mark all non-listed members of an enum default to the empty type is compact - "I told you the enum, so you are permitted to fill in an empty type with every branch that does not actually need further members; and I had to opt in to that behavior, so that I purposefully get an error if I did not opt in but forgot an enum member". But if the enum is likely to change, it can lead to forgotten additions later on - "I'm adding member 'bar' to an enum that already has member 'foo'; therefore, grepping for 'foo' should tell me all places where I should add handling for 'bar', except that it doesn't work when handling for 'foo' was implicit but handling for 'bar' should not be". Having said that, my personal preference is that opting in to an explicit way to do less typing ("all branches of the enum listed here are valid, and add no further members") seems like a nice syntactic sugar trick; and the easiest way to actually implement it is to probably already have support for an explicit 'foo':{} branch notation. That way, you can always change your mind later and undo the opt-in "data-partial" flag and rewrite the union with explicit empty branches when needed, to match how the sugar was expanded on your behalf.
Eric Blake <eblake@redhat.com> writes: > On 05/14/2018 01:08 PM, Markus Armbruster wrote: >> Anton Nefedov <anton.nefedov@virtuozzo.com> writes: >> >>> The patch makes possible to avoid introducing dummy empty types >>> for the flat union branches that have no data. >>> > >> >> Some unions have no variant members for certain discriminator values. >> We currently have to use an empty type there, and we've accumulated >> several just for the purpose, which is annoying. >> >> QAPI language design alternatives: >> >> 1. Having unions cover all discriminator values explicitly is useful. >> All we need is a more convenient way to denote empty types. Eric >> proposed {}, as in 'foo: {}. > > And even posted a patch for it once: > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00311.html > > although it was late enough in that series with other churn in the > meantime that it would need serious revisiting to apply today. > >> >> 2. Having unions repeat all the discriminator values explicitly is not >> useful. All we need is replacing the code enforcing that by code >> defaulting missing ones to the empty type. >> >> 3. We can't decide, so we do both (this patch). >> >> Preferences? > > Here's some tradeoffs to consider: > > Allowing 'foo':{} makes your intent explicit - "I know this branch of > the union exists, but it specifically adds no further members". Yes. > But > it's a lot of redundant typing - "I already had to type all the branch > names when declaring the enum type, why do it again here?" Redundancy isn't bad per se, but it needs to serve a useful purpose. What's the purpose of repeating the tag values? You give one below: grep. > Allowing an easy way to mark all non-listed members of an enum default > to the empty type is compact - "I told you the enum, so you are > permitted to fill in an empty type with every branch that does not > actually need further members; and I had to opt in to that behavior, > so that I purposefully get an error if I did not opt in but forgot an > enum member". Syntactic options aren't bad per se, but they need to serve a useful purpose. What's the purpose of having both unions that require all tag values, and unions that default omitted tag values to "no variant members"? > But if the enum is likely to change, it can lead to > forgotten additions later on - "I'm adding member 'bar' to an enum > that already has member 'foo'; therefore, grepping for 'foo' should > tell me all places where I should add handling for 'bar', except that > it doesn't work when handling for 'foo' was implicit but handling for > 'bar' should not be". If your code still compiles when you forget to add members to a struct or union, you obviously don't need the members you forgot :) For what it's worth, grepping for the enum type's name finds the union just fine, and is less likely to find unrelated stuff. > Having said that, my personal preference is that opting in to an > explicit way to do less typing ("all branches of the enum listed here > are valid, and add no further members") seems like a nice syntactic > sugar trick; and the easiest way to actually implement it is to > probably already have support for an explicit 'foo':{} branch > notation. That way, you can always change your mind later and undo > the opt-in "data-partial" flag and rewrite the union with explicit > empty branches when needed, to match how the sugar was expanded on > your behalf. Is that 1. combined with 3.? I dislike 3. because I feel it adds more complexity than the other options. More apparent once you add the necessary test coverage (another reason why requiring test coverage is such a good idea; having to cover every bell & whistle tends to make people reconsider which ones they actually need). I'd prefer a more opinionated design here. Either we opine making people repeat the tag values is an overall win. Then make them repeat them always, don't give them the option to not repeat them. Drop this patch. To reduce verbosity, we can add a suitable way to denote a predefined empty type. Or we opine it's not. Then let them not repeat them, don't give them the option to make themselves repeat them. Evolve this patch into one that makes flat union variants optional and default to empty. If we later find we still want a way to denote a predefined empty type, we can add it then. My personal opinion is it's not.
On 05/15/2018 02:01 AM, Markus Armbruster wrote: >>> QAPI language design alternatives: >>> >>> 1. Having unions cover all discriminator values explicitly is useful. >>> 2. Having unions repeat all the discriminator values explicitly is not >>> useful. All we need is replacing the code enforcing that by code >>> defaulting missing ones to the empty type. >>> >>> 3. We can't decide, so we do both (this patch). >>> > I'd prefer a more opinionated design here. > > Either we opine making people repeat the tag values is an overall win. > Then make them repeat them always, don't give them the option to not > repeat them. Drop this patch. To reduce verbosity, we can add a > suitable way to denote a predefined empty type. > > Or we opine it's not. Then let them not repeat them, don't give them > the option to make themselves repeat them. Evolve this patch into one > that makes flat union variants optional and default to empty. If we > later find we still want a way to denote a predefined empty type, we can > add it then. > > My personal opinion is it's not. I followed the arguments up to the last sentence, but then I got lost on whether you meant: This patch is not an overall win, so let's drop it and keep status quo and/or implement a way to write 'branch':{} (option 1 above) or Forcing repetition is not an overall win, so let's drop that requirement by using something similar to this patch (option 2 above) but without adding a 'partial-data' key. But you've convinced me that option 3 (supporting a compact branch representation AND supporting missing branches as defaulting to an empty type) is more of a maintenance burden (any time there is more than one way to write something, it requires more testing that both ways continue to work) and thus not worth doing without strong evidence that we need both ways (which we do not currently have).
Eric Blake <eblake@redhat.com> writes: > On 05/15/2018 02:01 AM, Markus Armbruster wrote: > >>>> QAPI language design alternatives: >>>> >>>> 1. Having unions cover all discriminator values explicitly is useful. > >>>> 2. Having unions repeat all the discriminator values explicitly is not >>>> useful. All we need is replacing the code enforcing that by code >>>> defaulting missing ones to the empty type. >>>> >>>> 3. We can't decide, so we do both (this patch). >>>> > >> I'd prefer a more opinionated design here. >> >> Either we opine making people repeat the tag values is an overall win. >> Then make them repeat them always, don't give them the option to not >> repeat them. Drop this patch. To reduce verbosity, we can add a >> suitable way to denote a predefined empty type. >> >> Or we opine it's not. Then let them not repeat them, don't give them >> the option to make themselves repeat them. Evolve this patch into one >> that makes flat union variants optional and default to empty. If we >> later find we still want a way to denote a predefined empty type, we can >> add it then. >> >> My personal opinion is it's not. > > I followed the arguments up to the last sentence, but then I got lost > on whether you meant: > > This patch is not an overall win, so let's drop it and keep status quo > and/or implement a way to write 'branch':{} (option 1 above) > > or > > Forcing repetition is not an overall win, so let's drop that > requirement by using something similar to this patch (option 2 above) > but without adding a 'partial-data' key. Sorry about that. I meant the latter. > But you've convinced me that option 3 (supporting a compact branch > representation AND supporting missing branches as defaulting to an > empty type) is more of a maintenance burden (any time there is more > than one way to write something, it requires more testing that both > ways continue to work) and thus not worth doing without strong > evidence that we need both ways (which we do not currently have).
On 15/5/2018 8:40 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 05/15/2018 02:01 AM, Markus Armbruster wrote: >> >>>>> QAPI language design alternatives: >>>>> >>>>> 1. Having unions cover all discriminator values explicitly is useful. >> >>>>> 2. Having unions repeat all the discriminator values explicitly is not >>>>> useful. All we need is replacing the code enforcing that by code >>>>> defaulting missing ones to the empty type. >>>>> >>>>> 3. We can't decide, so we do both (this patch). >>>>> >> >>> I'd prefer a more opinionated design here. >>> >>> Either we opine making people repeat the tag values is an overall win. >>> Then make them repeat them always, don't give them the option to not >>> repeat them. Drop this patch. To reduce verbosity, we can add a >>> suitable way to denote a predefined empty type. >>> >>> Or we opine it's not. Then let them not repeat them, don't give them >>> the option to make themselves repeat them. Evolve this patch into one >>> that makes flat union variants optional and default to empty. If we >>> later find we still want a way to denote a predefined empty type, we can >>> add it then. >>> >>> My personal opinion is it's not. >> >> I followed the arguments up to the last sentence, but then I got lost >> on whether you meant: >> >> This patch is not an overall win, so let's drop it and keep status quo >> and/or implement a way to write 'branch':{} (option 1 above) >> >> or >> >> Forcing repetition is not an overall win, so let's drop that >> requirement by using something similar to this patch (option 2 above) >> but without adding a 'partial-data' key. > > Sorry about that. I meant the latter. > >> But you've convinced me that option 3 (supporting a compact branch >> representation AND supporting missing branches as defaulting to an >> empty type) is more of a maintenance burden (any time there is more >> than one way to write something, it requires more testing that both >> ways continue to work) and thus not worth doing without strong >> evidence that we need both ways (which we do not currently have). I agree that neither option 3 nor this patch are the best way to handle this, so it's 1 or 2. (2) sure brings some prettiness into jsons; I wonder when it might harm; e.g. a person adds another block driver: it would be difficult to get round BlockdevOptionsFoo, and what can be missed is something optional like BlockdevStatsFoo, which is harmless if left empty and probably would be made an empty branch anyway. The difference is that an empty branch is something one might notice during a review and object. I think I'd vote for 2 (never enforce all-branches coverage) as well.
Anton Nefedov <anton.nefedov@virtuozzo.com> writes: > On 15/5/2018 8:40 PM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> writes: >> >>> On 05/15/2018 02:01 AM, Markus Armbruster wrote: >>> >>>>>> QAPI language design alternatives: >>>>>> >>>>>> 1. Having unions cover all discriminator values explicitly is useful. >>> >>>>>> 2. Having unions repeat all the discriminator values explicitly is not >>>>>> useful. All we need is replacing the code enforcing that by code >>>>>> defaulting missing ones to the empty type. >>>>>> >>>>>> 3. We can't decide, so we do both (this patch). >>>>>> >>> >>>> I'd prefer a more opinionated design here. >>>> >>>> Either we opine making people repeat the tag values is an overall win. >>>> Then make them repeat them always, don't give them the option to not >>>> repeat them. Drop this patch. To reduce verbosity, we can add a >>>> suitable way to denote a predefined empty type. >>>> >>>> Or we opine it's not. Then let them not repeat them, don't give them >>>> the option to make themselves repeat them. Evolve this patch into one >>>> that makes flat union variants optional and default to empty. If we >>>> later find we still want a way to denote a predefined empty type, we can >>>> add it then. >>>> >>>> My personal opinion is it's not. >>> >>> I followed the arguments up to the last sentence, but then I got lost >>> on whether you meant: >>> >>> This patch is not an overall win, so let's drop it and keep status quo >>> and/or implement a way to write 'branch':{} (option 1 above) >>> >>> or >>> >>> Forcing repetition is not an overall win, so let's drop that >>> requirement by using something similar to this patch (option 2 above) >>> but without adding a 'partial-data' key. >> >> Sorry about that. I meant the latter. >> >>> But you've convinced me that option 3 (supporting a compact branch >>> representation AND supporting missing branches as defaulting to an >>> empty type) is more of a maintenance burden (any time there is more >>> than one way to write something, it requires more testing that both >>> ways continue to work) and thus not worth doing without strong >>> evidence that we need both ways (which we do not currently have). > > I agree that neither option 3 nor this patch are the best way to handle > this, so it's 1 or 2. > > (2) sure brings some prettiness into jsons; I wonder when it might harm; > e.g. a person adds another block driver: it would be difficult to get > round BlockdevOptionsFoo, and what can be missed is something > optional like BlockdevStatsFoo, which is harmless if left empty and > probably would be made an empty branch anyway. The difference is that > an empty branch is something one might notice during a review and > object. Yes, forcing reviewer attention is the one real advantage of 1. I can see. I agree with you that reviewers missing the "main" union (such as BlockdevOptions) is unlikely. Missing "secondary" unions is more plausible. Let's see how many of unions sharing a tag enumeration type we have. A quick hack to introspect.py coughs up: Flat union type Tag enum type -------------------------------------------------------------- BlockdevCreateOptions BlockdevDriver BlockdevOptions BlockdevDriver BlockdevQcow2Encryption BlockdevQcow2EncryptionFormat ImageInfoSpecificQCow2Encryption BlockdevQcow2EncryptionFormat BlockdevQcowEncryption BlockdevQcowEncryptionFormat CpuInfo CpuInfoArch GuestPanicInformation GuestPanicInformationType QCryptoBlockCreateOptions QCryptoBlockFormat SchemaInfo SchemaMetaType SheepdogRedundancy SheepdogRedundancyType SocketAddress SocketAddressType SshHostKeyCheck SshHostKeyCheckMode CpuInfoFast SysEmuTarget UsernetConnection UsernetType Two pairs. We'll live. > I think I'd vote for 2 (never enforce all-branches coverage) as well. Eric, what do you think? One more thought: if we ever get around to provide more convenient flat union syntax so users don't have to enumerate the variant names twice, we'll need a way to say "empty branch". Let's worry about that problem when we have it.
On 05/17/2018 03:05 AM, Markus Armbruster wrote: >>>>>>> QAPI language design alternatives: >>>>>>> >>>>>>> 1. Having unions cover all discriminator values explicitly is useful. >>>> >>>>>>> 2. Having unions repeat all the discriminator values explicitly is not >>>>>>> useful. All we need is replacing the code enforcing that by code >>>>>>> defaulting missing ones to the empty type. >>>>>>> >> I think I'd vote for 2 (never enforce all-branches coverage) as well. > > Eric, what do you think? I'm sold. Let's go ahead and make the change that for any flat union, a branch not listed defaults to the empty type (no added fields) rather than being an error, then simplify a couple of the existing flat unions that benefit from that. > > One more thought: if we ever get around to provide more convenient flat > union syntax so users don't have to enumerate the variant names twice, > we'll need a way to say "empty branch". Let's worry about that problem > when we have it. In other words, our current "simple" unions act in a manner that declares an implicit enum type - if we ever add an implicit enum to a flat union (where you don't have to declare a pre-existing enum type), then you need some syntax to declare additional acceptable enum values that form an empty branch. Indeed, not a problem to be solved right now.
Eric Blake <eblake@redhat.com> writes: > On 05/17/2018 03:05 AM, Markus Armbruster wrote: >>>>>>>> QAPI language design alternatives: >>>>>>>> >>>>>>>> 1. Having unions cover all discriminator values explicitly is useful. >>>>> >>>>>>>> 2. Having unions repeat all the discriminator values explicitly is not >>>>>>>> useful. All we need is replacing the code enforcing that by code >>>>>>>> defaulting missing ones to the empty type. >>>>>>>> > >>> I think I'd vote for 2 (never enforce all-branches coverage) as well. >> >> Eric, what do you think? > > I'm sold. Let's go ahead and make the change that for any flat union, > a branch not listed defaults to the empty type (no added fields) > rather than being an error, then simplify a couple of the existing > flat unions that benefit from that. Anton, would you like to give this a try? [...]
On 18/5/2018 9:45 AM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> writes: > >> On 05/17/2018 03:05 AM, Markus Armbruster wrote: >>>>>>>>> QAPI language design alternatives: >>>>>>>>> >>>>>>>>> 1. Having unions cover all discriminator values explicitly is useful. >>>>>> >>>>>>>>> 2. Having unions repeat all the discriminator values explicitly is not >>>>>>>>> useful. All we need is replacing the code enforcing that by code >>>>>>>>> defaulting missing ones to the empty type. >>>>>>>>> >> >>>> I think I'd vote for 2 (never enforce all-branches coverage) as well. >>> >>> Eric, what do you think? >> >> I'm sold. Let's go ahead and make the change that for any flat union, >> a branch not listed defaults to the empty type (no added fields) >> rather than being an error, then simplify a couple of the existing >> flat unions that benefit from that. > > Anton, would you like to give this a try? > > [...] > Sure, I can try this next week.
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py index a032cec..ec5cf28 100644 --- a/scripts/qapi/common.py +++ b/scripts/qapi/common.py @@ -721,6 +721,7 @@ def check_union(expr, info): name = expr['union'] base = expr.get('base') discriminator = expr.get('discriminator') + partial = expr.get('data-partial', False) members = expr['data'] # Two types of unions, determined by discriminator. @@ -783,7 +784,7 @@ def check_union(expr, info): % (key, enum_define['enum'])) # If discriminator is user-defined, ensure all values are covered - if enum_define: + if enum_define and not partial: for value in enum_define['data']: if value not in members.keys(): raise QAPISemError(info, "Union '%s' data missing '%s' branch" @@ -909,7 +910,7 @@ def check_exprs(exprs): elif 'union' in expr: meta = 'union' check_keys(expr_elem, 'union', ['data'], - ['base', 'discriminator']) + ['base', 'discriminator', 'data-partial']) union_types[expr[meta]] = expr elif 'alternate' in expr: meta = 'alternate' @@ -1035,7 +1036,7 @@ class QAPISchemaVisitor(object): def visit_array_type(self, name, info, element_type): pass - def visit_object_type(self, name, info, base, members, variants): + def visit_object_type(self, name, info, base, members, variants, partial): pass def visit_object_type_flat(self, name, info, members, variants): @@ -1192,7 +1193,8 @@ class QAPISchemaArrayType(QAPISchemaType): class QAPISchemaObjectType(QAPISchemaType): - def __init__(self, name, info, doc, base, local_members, variants): + def __init__(self, name, info, doc, base, local_members, variants, + partial = False): # struct has local_members, optional base, and no variants # flat union has base, variants, and no local_members # simple union has local_members, variants, and no base @@ -1209,6 +1211,7 @@ class QAPISchemaObjectType(QAPISchemaType): self.local_members = local_members self.variants = variants self.members = None + self.partial = partial def check(self, schema): if self.members is False: # check for cycles @@ -1269,7 +1272,8 @@ class QAPISchemaObjectType(QAPISchemaType): def visit(self, visitor): visitor.visit_object_type(self.name, self.info, - self.base, self.local_members, self.variants) + self.base, self.local_members, self.variants, + self.partial) visitor.visit_object_type_flat(self.name, self.info, self.members, self.variants) @@ -1636,6 +1640,7 @@ class QAPISchema(object): name = expr['union'] data = expr['data'] base = expr.get('base') + partial = expr.get('data-partial', False) tag_name = expr.get('discriminator') tag_member = None if isinstance(base, dict): @@ -1656,7 +1661,8 @@ class QAPISchema(object): QAPISchemaObjectType(name, info, doc, base, members, QAPISchemaObjectTypeVariants(tag_name, tag_member, - variants))) + variants), + partial)) def _def_alternate_type(self, expr, info, doc): name = expr['alternate'] diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py index 9b312b2..40dffc4 100644 --- a/scripts/qapi/doc.py +++ b/scripts/qapi/doc.py @@ -211,7 +211,7 @@ class QAPISchemaGenDocVisitor(qapi.common.QAPISchemaVisitor): body=texi_entity(doc, 'Values', member_func=texi_enum_value))) - def visit_object_type(self, name, info, base, members, variants): + def visit_object_type(self, name, info, base, members, variants, partial): doc = self.cur_doc if base and base.is_implicit(): base = None diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py index 64d9c0f..e805509 100644 --- a/scripts/qapi/types.py +++ b/scripts/qapi/types.py @@ -215,7 +215,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor): self._genh.add(gen_array(name, element_type)) self._gen_type_cleanup(name) - def visit_object_type(self, name, info, base, members, variants): + def visit_object_type(self, name, info, base, members, variants, partial): # Nothing to do for the special empty builtin if name == 'q_empty': return diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py index 5d72d89..3ee64bb 100644 --- a/scripts/qapi/visit.py +++ b/scripts/qapi/visit.py @@ -34,7 +34,7 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp); c_name=c_name(name)) -def gen_visit_object_members(name, base, members, variants): +def gen_visit_object_members(name, base, members, variants, partial): ret = mcgen(''' void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) @@ -93,9 +93,10 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp) ret += mcgen(''' default: - abort(); + %(action)s } -''') +''', + action="break;" if partial else "abort();") # 'goto out' produced for base, for each member, and if variants were # present @@ -309,12 +310,13 @@ class QAPISchemaGenVisitVisitor(QAPISchemaModularCVisitor): self._genh.add(gen_visit_decl(name)) self._genc.add(gen_visit_list(name, element_type)) - def visit_object_type(self, name, info, base, members, variants): + def visit_object_type(self, name, info, base, members, variants, partial): # Nothing to do for the special empty builtin if name == 'q_empty': return self._genh.add(gen_visit_members_decl(name)) - self._genc.add(gen_visit_object_members(name, base, members, variants)) + self._genc.add(gen_visit_object_members(name, base, members, variants, + partial)) # TODO Worth changing the visitor signature, so we could # directly use rather than repeat type.is_implicit()? if not name.startswith('q_'): diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index c1a144b..95cd575 100644 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -28,7 +28,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): if prefix: print(' prefix %s' % prefix) - def visit_object_type(self, name, info, base, members, variants): + def visit_object_type(self, name, info, base, members, variants, partial): print('object %s' % name) if base: print(' base %s' % base.name)
The patch makes possible to avoid introducing dummy empty types for the flat union branches that have no data. Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> --- scripts/qapi/common.py | 18 ++++++++++++------ scripts/qapi/doc.py | 2 +- scripts/qapi/types.py | 2 +- scripts/qapi/visit.py | 12 +++++++----- tests/qapi-schema/test-qapi.py | 2 +- 5 files changed, 22 insertions(+), 14 deletions(-)