Re: [Qemu-devel] [PATCH v9 27/37] qapi: Add type.is_empty() helper

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> And use it in qapi-types and qapi-event.  Down the road, we may
> 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.  More
> immediately, the new .is_empty() helper will help fix a bug in
> qapi-visit in the next patch, where the generator did not handle
> an explicit but empty type in the same was as a missing type.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: improve commit message
> v8: no change
> v7: rebase to context change
> v6: new patch
> ---
>  scripts/qapi-event.py | 6 +++---
>  scripts/qapi-types.py | 2 +-
>  scripts/qapi.py       | 3 +++
>  3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index a1dca29..56c93a8 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -39,7 +39,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():

With .is_empty() expanded and negations folded:

       if arg_type and (arg_type.members or arg_type.variants):

Effectively the same, because .variants is None here.  Asserted in
QAPISchemaEvent.check().

>          ret += mcgen('''
>      QmpOutputVisitor *qov;
>      Visitor *v;
> @@ -58,7 +58,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():

Likewise.

However, we still use arg_type.members:

>          ret += mcgen('''
>      qov = qmp_output_visitor_new();
>      v = qmp_output_get_visitor(qov);

       visit_start_struct(v, "%(name)s", NULL, 0, &err);
   ''',
                        name=name)
           ret += gen_err_check()
           ret += gen_visit_fields(arg_type.members, need_cast=True,
                                   label='out_obj')
           ret += mcgen('''
   out_obj:
       visit_end_struct(v, &err);
       if (err) {
           goto out;
       }

Your patch makes liberates the guard from the implementation, but not
the actual use.  That's okay.  Should we comment or assert that variants
aren't implemented here, even though we already assert in
QAPISchemaEvent.check()?

> @@ -88,7 +88,7 @@ out_obj:
>  ''',
>                   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:
>      qmp_output_visitor_cleanup(qov);
> diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
> index d3f631a..c70fae1 100644
> --- a/scripts/qapi-types.py
> +++ b/scripts/qapi-types.py
> @@ -76,7 +76,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_field_for_empty_struct;
>  ''')

This is similar: effectively the same because base.variants is None.

I'm pretty sure we check base is a plain struct (no variants) somewhere.
I think we should also assert not base.variants in
QAPISchemaObjectType.check(), for completeness and clarity.

> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 3b4c62e..a13c110 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -973,6 +973,9 @@ class QAPISchemaObjectType(QAPISchemaType):
>          # See QAPISchema._make_implicit_object_type()
>          return self.name[0] == ':'
>
> +    def is_empty(self):
> +        return not self.members and not self.variants
> +

Works only after .check().  Before .check(), self.members is None.  Your
uses are all safely after .check().

Suggest to assert self.members is not None.

>      def c_name(self):
>          assert not self.is_implicit()
>          return QAPISchemaType.c_name(self)