Eric Blake <ebl...@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 <ebl...@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?
