Hi
----- Original Message -----
> Marc-André Lureau <marcandre.lur...@redhat.com> writes:
>
> > Take 'if' from expression, and use it to construct entity objects.
> > Shared implicit objects must share the same 'if' condition.
>
> Shared by what?
Shared by various make_implicit_object_type() users.
>
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > scripts/qapi.py | 81
> > +++++++++++++++++++++++++++++++++++++--------------------
> > 1 file changed, 53 insertions(+), 28 deletions(-)
> >
> > diff --git a/scripts/qapi.py b/scripts/qapi.py
> > index a94d93ada4..dc40d74abb 100644
> > --- a/scripts/qapi.py
> > +++ b/scripts/qapi.py
> > @@ -992,7 +992,7 @@ def check_exprs(exprs):
> > #
> >
> > class QAPISchemaEntity(object):
> > - def __init__(self, name, info, doc):
> > + def __init__(self, name, info, doc, ifcond=None):
> > assert isinstance(name, str)
> > self.name = name
> > # For explicitly defined entities, info points to the (explicit)
>
> Not visible here: QAPISchemaType inherits this.
>
> > @@ -1002,6 +1002,7 @@ class QAPISchemaEntity(object):
> > # such place).
> > self.info = info
> > self.doc = doc
> > + self.ifcond = ifcond
> >
> > def c_name(self):
> > return c_name(self.name)
> > @@ -1118,8 +1119,8 @@ class QAPISchemaBuiltinType(QAPISchemaType):
> >
> >
> > class QAPISchemaEnumType(QAPISchemaType):
> > - def __init__(self, name, info, doc, values, prefix):
> > - QAPISchemaType.__init__(self, name, info, doc)
> > + def __init__(self, name, info, doc, values, prefix, ifcond=None):
> > + QAPISchemaType.__init__(self, name, info, doc, ifcond)
> > for v in values:
> > assert isinstance(v, QAPISchemaMember)
> > v.set_owner(name)
> > @@ -1162,6 +1163,7 @@ class QAPISchemaArrayType(QAPISchemaType):
> > def check(self, schema):
> > self.element_type = schema.lookup_type(self._element_type_name)
> > assert self.element_type
> > + self.ifcond = self.element_type.ifcond
>
> This is subtler than it looks on first glance.
>
> All the other entities set self.ifcond in their constructor to the true
> value passed in as argument.
>
> QAPISchemaArrayType doesn't take such an argument. Instead, it inherits
> its .ifcond from its .element_type. However, .element_type isn't known
> at construction time if it's a forward reference. We therefore delay
> setting it until .check() time. You do the same for .ifcond (no
> choice).
>
> Before .check(), .ifcond is None, because the constructor sets it that
> way: it calls QAPISchemaType.__init__() without passing a ifcond
> argument, which then sets self.ifcond to its default argument None.
>
> Pitfall: accessing ent.ifcond before ent.check() works *except* when ent
> is an array type. Hmm.
>
> >
> > def is_implicit(self):
> > return True
> > @@ -1183,11 +1185,12 @@ class QAPISchemaArrayType(QAPISchemaType):
> >
> >
> > class QAPISchemaObjectType(QAPISchemaType):
> > - def __init__(self, name, info, doc, base, local_members, variants):
> > + def __init__(self, name, info, doc, base, local_members, variants,
> > + ifcond=None):
> > # struct has local_members, optional base, and no variants
> > # flat union has base, variants, and no local_members
> > # simple union has local_members, variants, and no base
> > - QAPISchemaType.__init__(self, name, info, doc)
> > + QAPISchemaType.__init__(self, name, info, doc, ifcond)
> > assert base is None or isinstance(base, str)
> > for m in local_members:
> > assert isinstance(m, QAPISchemaObjectTypeMember)
> > @@ -1375,8 +1378,8 @@ class
> > QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
> >
> >
> > class QAPISchemaAlternateType(QAPISchemaType):
> > - def __init__(self, name, info, doc, variants):
> > - QAPISchemaType.__init__(self, name, info, doc)
> > + def __init__(self, name, info, doc, variants, ifcond):
> > + QAPISchemaType.__init__(self, name, info, doc, ifcond)
> > assert isinstance(variants, QAPISchemaObjectTypeVariants)
> > assert variants.tag_member
> > variants.set_owner(name)
> > @@ -1413,8 +1416,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
> >
> > class QAPISchemaCommand(QAPISchemaEntity):
> > def __init__(self, name, info, doc, arg_type, ret_type,
> > - gen, success_response, boxed):
> > - QAPISchemaEntity.__init__(self, name, info, doc)
> > + gen, success_response, boxed, ifcond):
> > + QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
> > assert not arg_type or isinstance(arg_type, str)
> > assert not ret_type or isinstance(ret_type, str)
> > self._arg_type_name = arg_type
> > @@ -1451,8 +1454,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
> >
> >
> > class QAPISchemaEvent(QAPISchemaEntity):
> > - def __init__(self, name, info, doc, arg_type, boxed):
> > - QAPISchemaEntity.__init__(self, name, info, doc)
> > + def __init__(self, name, info, doc, arg_type, boxed, ifcond):
> > + QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
> > assert not arg_type or isinstance(arg_type, str)
> > self._arg_type_name = arg_type
> > self.arg_type = None
>
> Hmm. You're adding parameter ifcond to all entity constructors. The
> entity constructors' signatures all look like this:
>
> def __init__(<common parameters>, <specific parameters>)
>
> where <common parameters> is self, name, info, doc.
>
> Since ifcond is common, let's add it to <common parameters>. Pick a
> position you like.
>
ok
> > @@ -1547,11 +1550,11 @@ class QAPISchema(object):
> > def _make_enum_members(self, values):
> > return [QAPISchemaMember(v) for v in values]
> >
> > - def _make_implicit_enum_type(self, name, info, values):
> > + def _make_implicit_enum_type(self, name, info, values, ifcond):
> > # See also QAPISchemaObjectTypeMember._pretty_owner()
> > name = name + 'Kind' # Use namespace reserved by add_name()
> > self._def_entity(QAPISchemaEnumType(
> > - name, info, None, self._make_enum_members(values), None))
> > + name, info, None, self._make_enum_members(values), None,
> > ifcond))
> > return name
>
> Called by _def_union_type() to create the implicit tag enum, with the
> same ifcond it passes to QAPISchemaObjectType(). Thus, the implicit
> type's ifcond matches the ifcond of its only user. Good.
>
> >
> > def _make_array_type(self, element_type, info):
> > @@ -1560,22 +1563,29 @@ class QAPISchema(object):
> > self._def_entity(QAPISchemaArrayType(name, info,
> > element_type))
> > return name
> >
> > - def _make_implicit_object_type(self, name, info, doc, role, members):
> > + def _make_implicit_object_type(self, name, info, doc, role, members,
> > + ifcond=None):
> > if not members:
> > return None
> > # See also QAPISchemaObjectTypeMember._pretty_owner()
> > name = 'q_obj_%s-%s' % (name, role)
> > - if not self.lookup_entity(name, QAPISchemaObjectType):
> > + typ = self.lookup_entity(name, QAPISchemaObjectType)
> > + if typ:
> > + # FIXME: generating the disjunction of all conditions
>
> What are "all conditions"? I hope I can shed some light on that below.
>
> > + assert ifcond == typ.ifcond
> > + else:
> > self._def_entity(QAPISchemaObjectType(name, info, doc, None,
> > - members, None))
> > + members, None, ifcond))
> > return name
>
> Several callers:
>
> * _make_simple_variant() to create a wrapper type, with the wrapped
> type's ifcond. The wrapped type's ifcond is necessary for the wrapper
> type to compile. It's not sufficient for it to be needed. The
> "needed" condition is the disjunction of all users' ifcond.
>
> In other words, your code generates condition that is correct, but not
> tight. Your FIXME is about tightening it. I'd make it a TODO,
> because nothing is actually broken now.
>
> Need a comment explaining this. Perhaps:
>
> if typ:
> # The implicit object type has multiple users. This can
> # happen only for simple unions' implicit wrapper types.
> # Its ifcond should be the disjunction of its user's
> # ifconds. Not implemented. Instead, we always pass the
> # wrapped type's ifcond, which is trivially the same for all
> # users. It's also necessary for the wrapper to compile.
> # But it's not tight: the disjunction need not imply it. We
> # may end up compiling useless wrapper types.
> # TODO kill simple unions or implement the disjunction
>
> I hate simple unions.
ok
>
> * _def_union_type() to create an implicit base type, with same ifcond it
> passes to QAPISchemaObjectType(). Thus, the implicit base type's
> ifcond matches the ifcond of its only user. Good.
>
> * _def_command() to create an implicit argument type, with the same
> ifcond it passes to QAPISchemaCommand(). Thus, the implicit argument
> type's ifcond matches the ifcond of its only user. Good.
>
> * _def_event() likewise.
>
> I still can't make sense of the commit message's "Shared implicit
> objects must share the same 'if' condition."
>
> >
> > def _def_enum_type(self, expr, info, doc):
> > name = expr['enum']
> > data = expr['data']
> > prefix = expr.get('prefix')
> > + ifcond = expr.get('if')
> > self._def_entity(QAPISchemaEnumType(
> > - name, info, doc, self._make_enum_members(data), prefix))
> > + name, info, doc, self._make_enum_members(data), prefix,
> > + ifcond))
> >
> > def _make_member(self, name, typ, info):
> > optional = False
> > @@ -1595,9 +1605,10 @@ class QAPISchema(object):
> > name = expr['struct']
> > base = expr.get('base')
> > data = expr['data']
> > + ifcond = expr.get('if')
> > self._def_entity(QAPISchemaObjectType(name, info, doc, base,
> > self._make_members(data,
> > info),
> > - None))
> > + None, ifcond))
> >
> > def _make_variant(self, case, typ):
> > return QAPISchemaObjectTypeVariant(case, typ)
> > @@ -1606,19 +1617,23 @@ class QAPISchema(object):
> > if isinstance(typ, list):
> > assert len(typ) == 1
> > typ = self._make_array_type(typ[0], info)
> > + type_entity = self.lookup_type(typ)
> > typ = self._make_implicit_object_type(
> > - typ, info, None, 'wrapper', [self._make_member('data', typ,
> > info)])
> > + typ, info, None, 'wrapper',
> > + [self._make_member('data', typ, info)], type_entity.ifcond)
> > return QAPISchemaObjectTypeVariant(case, typ)
>
> This is where you create the wrapper type with the wrapped type's
> ifcond.
>
> I don't like the name type_entity. I'd simply eliminate the variable.
>
ok
> >
> > def _def_union_type(self, expr, info, doc):
> > name = expr['union']
> > data = expr['data']
> > base = expr.get('base')
> > + ifcond = expr.get('if')
> > tag_name = expr.get('discriminator')
> > tag_member = None
> > if isinstance(base, dict):
> > - base = (self._make_implicit_object_type(
> > - name, info, doc, 'base', self._make_members(base, info)))
> > + base = self._make_implicit_object_type(
> > + name, info, doc, 'base', self._make_members(base, info),
> > + ifcond)
> > if tag_name:
> > variants = [self._make_variant(key, value)
> > for (key, value) in data.iteritems()]
>
> This is where you create a union's implicit base type with the union's
> ifcond.
>
> > @@ -1627,18 +1642,21 @@ class QAPISchema(object):
> > variants = [self._make_simple_variant(key, value, info)
> > for (key, value) in data.iteritems()]
> > typ = self._make_implicit_enum_type(name, info,
> > - [v.name for v in
> > variants])
> > + [v.name for v in
> > variants],
> > + ifcond)
> > tag_member = QAPISchemaObjectTypeMember('type', typ, False)
> > members = [tag_member]
>
> This is where you create a union's implicit tag enum with the union's ifcond.
>
> > self._def_entity(
> > QAPISchemaObjectType(name, info, doc, base, members,
> > QAPISchemaObjectTypeVariants(tag_name,
> > tag_member,
> > - variants)))
> > + variants),
> > + ifcond))
> >
> > def _def_alternate_type(self, expr, info, doc):
> > name = expr['alternate']
> > data = expr['data']
> > + ifcond = expr.get('if')
> > variants = [self._make_variant(key, value)
> > for (key, value) in data.iteritems()]
> > tag_member = QAPISchemaObjectTypeMember('type', 'QType', False)
> > @@ -1646,7 +1664,8 @@ class QAPISchema(object):
> > QAPISchemaAlternateType(name, info, doc,
> > QAPISchemaObjectTypeVariants(None,
> >
> > tag_member,
> > -
> > variants)))
> > +
> > variants),
> > + ifcond))
> >
> > def _def_command(self, expr, info, doc):
> > name = expr['command']
> > @@ -1655,23 +1674,29 @@ class QAPISchema(object):
> > gen = expr.get('gen', True)
> > success_response = expr.get('success-response', True)
> > boxed = expr.get('boxed', False)
> > + ifcond = expr.get('if')
> > if isinstance(data, OrderedDict):
> > data = self._make_implicit_object_type(
> > - name, info, doc, 'arg', self._make_members(data, info))
> > + name, info, doc, 'arg', self._make_members(data, info),
> > + ifcond)
>
> This is where you create a command's implicit argument type with the
> command's ifcond.
>
> > if isinstance(rets, list):
> > assert len(rets) == 1
> > rets = self._make_array_type(rets[0], info)
> > self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
> > - gen, success_response, boxed))
> > + gen, success_response, boxed,
> > + ifcond))
> >
> > def _def_event(self, expr, info, doc):
> > name = expr['event']
> > data = expr.get('data')
> > boxed = expr.get('boxed', False)
> > + ifcond = expr.get('if')
> > if isinstance(data, OrderedDict):
> > data = self._make_implicit_object_type(
> > - name, info, doc, 'arg', self._make_members(data, info))
> > - self._def_entity(QAPISchemaEvent(name, info, doc, data, boxed))
> > + name, info, doc, 'arg', self._make_members(data, info),
> > + ifcond)
>
> This is where you create an event's implicit argument type with the
> event's ifcond.
>
> > + self._def_entity(QAPISchemaEvent(name, info, doc, data, boxed,
> > + ifcond))
> >
> > def _def_exprs(self):
> > for expr_elem in self.exprs:
>
