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

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3584 bytes --]

Hi

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

> 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.
>
>
I dropped the patch, I didn't realize it wasn't necessary anymore.

[-- Attachment #2: Type: text/html, Size: 5117 bytes --]