Am 26.07.2013 um 15:40 hat Eric Blake geschrieben: > On 07/23/2013 07:03 AM, Kevin Wolf wrote: > > Instead of the rather verbose syntax that distinguishes base and > > subclass fields... > > > > { "type": "file", > > "read-only": true, > > "data": { > > "filename": "test" > > } } > > > > ...we can now have both in the same namespace, allowing a more direct > > mapping of the command line, and moving fields between the common base > > and subclasses without breaking the API: > > > > { "driver": "file", > > "read-only": true, > > "filename": "test" } > > MUCH nicer! > > > > > Signed-off-by: Kevin Wolf <kw...@redhat.com> > > --- > > docs/qapi-code-gen.txt | 22 +++++++++++++ > > scripts/qapi-types.py | 6 ++++ > > scripts/qapi-visit.py | 86 > > ++++++++++++++++++++++++++++++++++++-------------- > > 3 files changed, 91 insertions(+), 23 deletions(-) > > > > > diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt > > index 555ca66..c187fda 100644 > > --- a/docs/qapi-code-gen.txt > > +++ b/docs/qapi-code-gen.txt > > @@ -103,6 +103,28 @@ And it looks like this on the wire: > > "data" : { "backing-file": "/some/place/my-image", > > "lazy-refcounts": true } } > > > > + > > +Flat union types avoid the nesting on the wire. They are used whenever a > > +specific field of the base type is declared as the discriminator ('type' is > > +then no longer generated). The discriminator must always be a string field. > > Since an 'enum' is always sent over the wire as a string, is it > appropriate to allow the discriminator to be an enum field instead of a > 'str'? But that could be done in a followup patch; your initial usage > is just fine with 'str'. Besides, if we allow the discriminator to have > 'enum' type, that would imply that we want to guarantee coverage that > all of the 'data' members of the union type correspond to the members of > the union. On the other hand, that would be extra type safety - if we > extend the enum that backs the discriminator, we'd immediately be > reminded if we forgot to also extend the union based on that enum. > Again, food for thought for a future patch, and not something needed for > this one.
There's nothing to add to this, I would certainly support such a future patch. > > +The above example can then be modified as follows: > > + > > + { 'type': 'BlockdevCommonOptions', > > + 'data': { 'driver': 'str' 'readonly': 'bool' } } > > Missing a comma. > > > +++ b/scripts/qapi-types.py > > @@ -161,7 +161,9 @@ def generate_union(expr): > > > > name = expr['union'] > > typeinfo = expr['data'] > > + > > base = expr.get('base') > > + discriminator = expr.get('discriminator') > > > > ret = mcgen(''' > > struct %(name)s > > @@ -185,7 +187,11 @@ struct %(name)s > > > > if base: > > struct = find_struct(base) > > + if discriminator: > > + del struct['data'][discriminator] > > I asked before, but didn't get an answer; my question may just show my > unfamiliarity with python. Is this modifying the original 'struct', > such that other uses of the struct after this point will no longer > contain the discriminator key? Or is it only modifying a copy of > 'struct', with the original left intact? But based on the rest of your > patch... Sorry, this is in fact my own unfamiliarity with Python, combined with failure to fix all cases when you pointed it out. The only reason I didn't reply to that part of your review was that I thought it would be obvious when I send a fixed version. Well, except if I don't. I've changed this hunk now to match the other one: @@ -184,8 +186,13 @@ struct %(name)s ''') if base: - struct = find_struct(base) - ret += generate_struct_fields(struct['data']) + base_fields = find_struct(base)['data'] + if discriminator: + base_fields = base_fields.copy() + del base_fields[discriminator] + ret += generate_struct_fields(base_fields) + else: + assert not discriminator ret += mcgen(''' }; > I think my findings are easy fixes; so I'm okay if you fix them and then > add: > > Reviewed-by: Eric Blake <ebl...@redhat.com> Thanks, Eric. Kevin