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.