Re: [PATCH v6 03/11] qapi: add QAPISchemaIfCond.is_present()

[Marc-André Lureau]

On Mon, Aug 2, 2021 at 1:52 PM Markus Armbruster <armbru@redhat.com> wrote:

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  docs/sphinx/qapidoc.py         | 8 ++++----
>  scripts/qapi/schema.py         | 7 +++++--
>  tests/qapi-schema/test-qapi.py | 2 +-
>  3 files changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 0eac3308b2..511520f33f 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -139,7 +139,7 @@ def _nodes_for_one_member(self, member):
>              term.append(nodes.literal('', member.type.doc_type()))
>          if member.optional:
>              term.append(nodes.Text(' (optional)'))
> -        if member.ifcond.ifcond:
> +        if member.ifcond.is_present():
>              term.extend(self._nodes_for_ifcond(member.ifcond))
>          return term
> 
> @@ -154,7 +154,7 @@ def _nodes_for_variant_when(self, variants, variant):
>                  nodes.literal('', variants.tag_member.name),
>                  nodes.Text(' is '),
>                  nodes.literal('', '"%s"' % variant.name)]
> -        if variant.ifcond.ifcond:
> +        if variant.ifcond.is_present():
>              term.extend(self._nodes_for_ifcond(variant.ifcond))
>          return term
> 
> @@ -209,7 +209,7 @@ def _nodes_for_enum_values(self, doc):
>          dlnode = nodes.definition_list()
>          for section in doc.args.values():
>              termtext = [nodes.literal('', section.member.name)]
> -            if section.member.ifcond.ifcond:
> +            if section.member.ifcond.is_present():
>                  termtext.extend(self._nodes_for_ifcond(section.member.ifcond))
>              # TODO drop fallbacks when undocumented members are outlawed
>              if section.text:
> @@ -277,7 +277,7 @@ def _nodes_for_sections(self, doc):
>      def _nodes_for_if_section(self, ifcond):
>          """Return list of doctree nodes for the "If" section"""
>          nodelist = []
> -        if ifcond.ifcond:
> +        if ifcond.is_present():
>              snode = self._make_section('If')
>              snode += nodes.paragraph(
>                  '', '', *self._nodes_for_ifcond(ifcond, with_if=False)
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 5e44164bd1..e3bd8f8720 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -29,6 +29,9 @@ class QAPISchemaIfCond:
>      def __init__(self, ifcond=None):
>          self.ifcond = ifcond or []
> 
> +    def is_present(self):
> +        return bool(self.ifcond)
> +
> 
>  class QAPISchemaEntity:
>      meta: Optional[str] = None
> @@ -599,7 +602,7 @@ def check(self, schema, seen):
>                      self.info,
>                      "discriminator member '%s' of %s must not be optional"
>                      % (self._tag_name, base))
> -            if self.tag_member.ifcond.ifcond:
> +            if self.tag_member.ifcond.is_present():
>                  raise QAPISemError(
>                      self.info,
>                      "discriminator member '%s' of %s must not be conditional"
> @@ -607,7 +610,7 @@ def check(self, schema, seen):
>          else:                   # simple union
>              assert isinstance(self.tag_member.type, QAPISchemaEnumType)
>              assert not self.tag_member.optional
> -            assert self.tag_member.ifcond.ifcond == []
> +            assert not self.tag_member.ifcond.is_present()
>          if self._tag_name:    # flat union
>              # branches that are not explicitly covered get an empty type
>              cases = {v.name for v in self.variants}
> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
> index 7907b4ac3a..c92be2d086 100755
> --- a/tests/qapi-schema/test-qapi.py
> +++ b/tests/qapi-schema/test-qapi.py
> @@ -94,7 +94,7 @@ def _print_variants(variants):
> 
>      @staticmethod
>      def _print_if(ifcond, indent=4):
> -        if ifcond.ifcond:
> +        if ifcond.is_present():
>              print('%sif %s' % (' ' * indent, ifcond.ifcond))
> 
>      @classmethod

In introspect.py:

        if obj.ifcond:
            ret += gen_if(obj.ifcond.ifcond)
        ret += _tree_to_qlit(obj.value, level)
        if obj.ifcond:
            ret += '\n' + gen_endif(obj.ifcond.ifcond)

I believe the previous patch should change it to

        if obj.ifcond.ifcond:
            ret += gen_if(obj.ifcond.ifcond)
        ret += _tree_to_qlit(obj.value, level)
        if obj.ifcond.ifcond:
            ret += '\n' + gen_endif(obj.ifcond.ifcond)

and this one to

        if obj.ifcond.is_present():
            ret += gen_if(obj.ifcond.ifcond)
        ret += _tree_to_qlit(obj.value, level)
        if obj.ifcond.is_present():
            ret += '\n' + gen_endif(obj.ifcond.ifcond)

done

Other than that:
Reviewed-by: Markus Armbruster <armbru@redhat.com>