diff mbox

[v7,05/15] qapi: Add type.is_empty() helper

Message ID 1463784024-17242-6-git-send-email-eblake@redhat.com
State New
Headers show

Commit Message

Eric Blake May 20, 2016, 10:40 p.m. UTC
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(-)

Comments

Markus Armbruster June 14, 2016, 2:01 p.m. UTC | #1
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 mbox

Patch

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;
 ''')