Message ID | 1463784024-17242-6-git-send-email-eblake@redhat.com |
---|---|
State | New |
Headers | show |
Eric Blake <eblake@redhat.com> writes: > And use it in qapi-types and qapi-event. In the near future, we > want to lift our artificial restriction of no variants at the > top level of an event, at which point, inlining our check for > whether members is empty will no longer be sufficient; but > adding an inline check for variants everywhere we are also looking > at members adds verbosity. For now, we already asserted that no > variants were present, so the conversion from 'if type.members' > to 'if not type.is_empty()' has no semantic change, but does > future-proof the code if we eventually do support variants in those > contexts. Suggest: qapi: Add type.is_empty() helper In the near future, we want to lift our artificial restriction of no variants at the top level of an event, at which point the currently open-coded check for empty members will become insufficient. Factor it out into new helper method is_empty() now. Future-proof it by checking variants, too. Since all current callers assert that there are no variants, this is no semantic change. > > No change to generated code. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v3: rebase to latest > [Previously posted as part of "easier unboxed visits/qapi implicit types"] > v2: no change > v1: add some asserts > [Previously posted as part of "qapi cleanup subset E"] > v9: improve commit message > v8: no change > v7: rebase to context change > v6: new patch > --- > scripts/qapi.py | 4 ++++ > scripts/qapi-event.py | 6 +++--- > scripts/qapi-types.py | 2 +- > 3 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/scripts/qapi.py b/scripts/qapi.py > index 3554ab1..90ea30c 100644 > --- a/scripts/qapi.py > +++ b/scripts/qapi.py > @@ -996,6 +996,10 @@ class QAPISchemaObjectType(QAPISchemaType): > # _def_predefineds() > return self.name.startswith('q_') > > + def is_empty(self): > + assert self.members is not None Not to be called before .check(). Good. > + return not self.members and not self.variants > + > def c_name(self): > return QAPISchemaType.c_name(self) > > diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py > index 9c88627..09c0a2a 100644 > --- a/scripts/qapi-event.py > +++ b/scripts/qapi-event.py > @@ -69,7 +69,7 @@ def gen_event_send(name, arg_type): Let's verify your claim "already asserted that no variants present". > ''', > proto=gen_event_send_proto(name, arg_type)) > > - if arg_type and arg_type.members: > + if arg_type and not arg_type.is_empty(): > ret += mcgen(''' > QObject *obj; > Visitor *v; ''') ret += gen_param_var(arg_type) gen_param_var() indeed asserts. > @@ -88,7 +88,7 @@ def gen_event_send(name, arg_type): > ''', > name=name) > > - if arg_type and arg_type.members: > + if arg_type and not arg_type.is_empty(): > ret += mcgen(''' > v = qmp_output_visitor_new(&obj); > > @@ -116,7 +116,7 @@ def gen_event_send(name, arg_type): > ''', > c_enum=c_enum_const(event_enum_name, name)) > > - if arg_type and arg_type.members: > + if arg_type and not arg_type.is_empty(): > ret += mcgen(''' > out: > visit_free(v); > diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py > index b3038e5..5a9e2da 100644 > --- a/scripts/qapi-types.py > +++ b/scripts/qapi-types.py > @@ -91,7 +91,7 @@ struct %(c_name)s { > # potential issues with attempting to malloc space for zero-length > # structs in C, and also incompatibility with C++ (where an empty > # struct is size 1). > - if not (base and base.members) and not members and not variants: > + if (not base or base.is_empty()) and not members and not variants: > ret += mcgen(''' > char qapi_dummy_for_empty_struct; > ''') I can't see an assertion here. qapi.py should reject use of base types with variants, though. Unless there is an assertion I missed, the commit message should be updated to be less specific. To check patch completeness, let's examine the other tests of .members. I can see just three more, all in gen_marshal(). Variants can't happen there (qapi.py should reject base types with variants), the code isn't prepared for them, and it asserts in gen_call(). Should we use .is_empty() there?
diff --git a/scripts/qapi.py b/scripts/qapi.py index 3554ab1..90ea30c 100644 --- a/scripts/qapi.py +++ b/scripts/qapi.py @@ -996,6 +996,10 @@ class QAPISchemaObjectType(QAPISchemaType): # _def_predefineds() return self.name.startswith('q_') + def is_empty(self): + assert self.members is not None + return not self.members and not self.variants + def c_name(self): return QAPISchemaType.c_name(self) diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py index 9c88627..09c0a2a 100644 --- a/scripts/qapi-event.py +++ b/scripts/qapi-event.py @@ -69,7 +69,7 @@ def gen_event_send(name, arg_type): ''', proto=gen_event_send_proto(name, arg_type)) - if arg_type and arg_type.members: + if arg_type and not arg_type.is_empty(): ret += mcgen(''' QObject *obj; Visitor *v; @@ -88,7 +88,7 @@ def gen_event_send(name, arg_type): ''', name=name) - if arg_type and arg_type.members: + if arg_type and not arg_type.is_empty(): ret += mcgen(''' v = qmp_output_visitor_new(&obj); @@ -116,7 +116,7 @@ def gen_event_send(name, arg_type): ''', c_enum=c_enum_const(event_enum_name, name)) - if arg_type and arg_type.members: + if arg_type and not arg_type.is_empty(): ret += mcgen(''' out: visit_free(v); diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index b3038e5..5a9e2da 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -91,7 +91,7 @@ struct %(c_name)s { # potential issues with attempting to malloc space for zero-length # structs in C, and also incompatibility with C++ (where an empty # struct is size 1). - if not (base and base.members) and not members and not variants: + if (not base or base.is_empty()) and not members and not variants: ret += mcgen(''' char qapi_dummy_for_empty_struct; ''')
And use it in qapi-types and qapi-event. In the near future, we want to lift our artificial restriction of no variants at the top level of an event, at which point, inlining our check for whether members is empty will no longer be sufficient; but adding an inline check for variants everywhere we are also looking at members adds verbosity. For now, we already asserted that no variants were present, so the conversion from 'if type.members' to 'if not type.is_empty()' has no semantic change, but does future-proof the code if we eventually do support variants in those contexts. No change to generated code. Signed-off-by: Eric Blake <eblake@redhat.com> --- v3: rebase to latest [Previously posted as part of "easier unboxed visits/qapi implicit types"] v2: no change v1: add some asserts [Previously posted as part of "qapi cleanup subset E"] v9: improve commit message v8: no change v7: rebase to context change v6: new patch --- scripts/qapi.py | 4 ++++ scripts/qapi-event.py | 6 +++--- scripts/qapi-types.py | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-)