Re: [PATCH v6 04/11] qapi: _make_enum_members() to work with pre-built QAPISchemaIfCond

From: Markus Armbruster <armbru@redhat.com>
To: marcandre.lureau@redhat.com
Cc: Eric Blake <eblake@redhat.com>,
	jsnow@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com
Subject: Re: [PATCH v6 04/11] qapi: _make_enum_members() to work with pre-built QAPISchemaIfCond
Date: Mon, 02 Aug 2021 12:41:12 +0200	[thread overview]
Message-ID: <874kc8ruwn.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20210618102507.3761128-5-marcandre.lureau@redhat.com> (marcandre lureau's message of "Fri, 18 Jun 2021 14:25:00 +0400")

marcandre.lureau@redhat.com writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Instead of lowering the expression back to its original form, and having
> to convert it again, special-case the 'if' condition to be pre-built.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  scripts/qapi/schema.py | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index e3bd8f8720..c35fa3bf51 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -982,8 +982,13 @@ def _make_features(self, features, info):
>                  for f in features]
>  
>      def _make_enum_members(self, values, info):
> -        return [QAPISchemaEnumMember(v['name'], info,
> -                                     QAPISchemaIfCond(v.get('if')))
> +        def _get_if(v):
> +            ifcond = v.get('if')
> +            if isinstance(ifcond, QAPISchemaIfCond):
> +                return ifcond
> +            else:
> +                return QAPISchemaIfCond(ifcond)
> +        return [QAPISchemaEnumMember(v['name'], info, _get_if(v))
>                  for v in values]
>  
>      def _make_implicit_enum_type(self, name, info, ifcond, values):
> @@ -1103,7 +1108,7 @@ def _def_union_type(self, expr, info, doc):
>                                            QAPISchemaIfCond(value.get('if')),
>                                            info)
>                  for (key, value) in data.items()]
> -            enum = [{'name': v.name, 'if': v.ifcond.ifcond} for v in variants]
> +            enum = [{'name': v.name, 'if': v.ifcond} for v in variants]
>              typ = self._make_implicit_enum_type(name, info, ifcond, enum)
>              tag_member = QAPISchemaObjectTypeMember('type', info, typ, False)
>              members = [tag_member]

I'm afraid I don't like this one.

Mapping from QAPISchemaIfCond back to the AST happens to be easy with
the current data structures, but you're right, it's not nice.

Stuffing the QAPISchemaIfCond into the AST is (in my opinion) worse:
it's a layering violation.

Let's take a step back and review what needs to be done here:

    for each simple union branch:
        create a simple variant
        create an implicit enum member
    and
        collect the variants in a list
        collect the enum members in a list

The code splits this work.  It first creates the list of variants from
the AST's simple union branches in @data:

            variants = [
                self._make_simple_variant(key, value['type'],
                                          QAPISchemaIfCond(value.get('if')),
                                          info)
                for (key, value) in data.items()]

It then creates the list of enum of enum members from the list of
variants, *not* from the AST:

            enum = [{'name': v.name, 'if': v.ifcond.ifcond} for v in variants]

This dots into the QAPISchemaVariant.  Your patch makes this dotting
less deep.

Two solutions I'd dislike less:

1. Create the enum members from the AST, too.

2. Do nothing, and bank on the eventual removal of simple unions.

Minimizing ripple effects on the remainder of the series is of course a
concern.