Hi
On Mon, Aug 2, 2021 at 2:41 PM Markus Armbruster <arm...@redhat.com> wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lur...@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.lur...@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.
