| Message ID | 1456443528-13901-14-git-send-email-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Eric Blake <eblake@redhat.com> writes: > Now that we have is_empty() and gen_visit_fields_call(), it's s/fields/members/. Better grep all the commit messages for "fields". > fairly easy to skip the visit of a variant type that has no > members. I figure that alone would've been just as easy before gen_visit_members_call(), but see below. > Only one such instance exists at the moment > (CpuInfoOther), My testing shows qapi-schema-test.json's Empty2 is also affected: @@ -198,7 +198,6 @@ void visit_type_Empty2_members(Visitor * { Error *err = NULL; - visit_type_Empty1_members(v, (Empty1 *)obj, &err); if (err) { goto out; } This isn't "the visit of a variant type that has no members", it's a visit of an empty base type. If you go back to PATCH 10, you can see this patch's stated purpose is tied to the last hunk there, which uses gen_visit_members_call() for a variant. Its other use is for the base. Suggest to rephrase this patch's commit message to capture both. > but the idea of a union where some branches > add no fields beyond the base type is common enough that we > may add syntax for other cases, as in > { 'union':'U', 'base':'B', 'discriminator':'D', > 'data': { 'default': {}, 'extra':'Type' } } I find 'default' confusing, because a variant record's default case is the case that applies to all the tag values not explicitly listed. What about simply 'case1' and 'case2'? > Note that the Abort type is another example of an empty type, > but it is only used by a simple union rather than a flat > union; and with simple unions, the wrapper type providing > the 'data' QMP key is not empty. As so often, simple unions are anything but. > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: no change > --- > scripts/qapi-visit.py | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py > index dbae00c..a9de393 100644 > --- a/scripts/qapi-visit.py > +++ b/scripts/qapi-visit.py > @@ -37,7 +37,9 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp); > def gen_visit_members_call(typ, c_name): > ret = '' > assert isinstance(typ, QAPISchemaObjectType) > - if typ.is_implicit(): > + if typ.is_empty(): > + pass > + elif typ.is_implicit(): > # TODO ugly special case for simple union > assert len(typ.members) == 1 > assert not typ.variants Okay because the patch is trivial. If it wasn't, I'd say avoiding the call isn't worthwhile; the compiler might even optimize it to nothing.
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index dbae00c..a9de393 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -37,7 +37,9 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp); def gen_visit_members_call(typ, c_name): ret = '' assert isinstance(typ, QAPISchemaObjectType) - if typ.is_implicit(): + if typ.is_empty(): + pass + elif typ.is_implicit(): # TODO ugly special case for simple union assert len(typ.members) == 1 assert not typ.variants
Now that we have is_empty() and gen_visit_fields_call(), it's fairly easy to skip the visit of a variant type that has no members. Only one such instance exists at the moment (CpuInfoOther), but the idea of a union where some branches add no fields beyond the base type is common enough that we may add syntax for other cases, as in { 'union':'U', 'base':'B', 'discriminator':'D', 'data': { 'default': {}, 'extra':'Type' } } Note that the Abort type is another example of an empty type, but it is only used by a simple union rather than a flat union; and with simple unions, the wrapper type providing the 'data' QMP key is not empty. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: no change --- scripts/qapi-visit.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)