qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v7 05/15] qapi: Add type.is_empty() helper


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v7 05/15] qapi: Add type.is_empty() helper
Date: Tue, 14 Jun 2016 16:01:26 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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?



reply via email to

[Prev in Thread] Current Thread [Next in Thread]