Hi
On Mon, Aug 2, 2021 at 1:21 PM Markus Armbruster <arm...@redhat.com> wrote:
> marcandre.lur...@redhat.com writes:
>
> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
> >
> > Except for the special casing assert in _make_implicit_object_type(),
> > which needs to handle schema objects, it's a mechanical change.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> > ---
> > docs/sphinx/qapidoc.py | 10 +++---
> > scripts/qapi/commands.py | 4 +--
> > scripts/qapi/events.py | 5 +--
> > scripts/qapi/gen.py | 14 ++++----
> > scripts/qapi/introspect.py | 26 +++++++-------
> > scripts/qapi/schema.py | 66 +++++++++++++++++++++-------------
> > scripts/qapi/types.py | 33 ++++++++---------
> > scripts/qapi/visit.py | 23 ++++++------
> > tests/qapi-schema/test-qapi.py | 4 +--
> > 9 files changed, 103 insertions(+), 82 deletions(-)
> >
> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> > index 87c67ab23f..0eac3308b2 100644
> > --- a/docs/sphinx/qapidoc.py
> > +++ b/docs/sphinx/qapidoc.py
> > @@ -116,7 +116,7 @@ def _nodes_for_ifcond(self, ifcond, with_if=True):
> > the conditions are in literal-text and the commas are not.
> > If with_if is False, we don't return the "(If: " and ")".
> > """
> > - condlist = intersperse([nodes.literal('', c) for c in ifcond],
> > + condlist = intersperse([nodes.literal('', c) for c in
> ifcond.ifcond],
>
> Mechanical pattern #1: ifcond becomes ifcond.ifcond to peel off the new
> wrapper. ifcond.ifcond is ugly, but almost all instances go away in
> this series. I'm okay with the remainder.
>
> > nodes.Text(', '))
> > if not with_if:
> > return condlist
> > @@ -139,7 +139,7 @@ def _nodes_for_one_member(self, member):
> > term.append(nodes.literal('', member.type.doc_type()))
> > if member.optional:
> > term.append(nodes.Text(' (optional)'))
> > - if member.ifcond:
> > + if member.ifcond.ifcond:
> > term.extend(self._nodes_for_ifcond(member.ifcond))
> > return term
> >
> > @@ -154,7 +154,7 @@ def _nodes_for_variant_when(self, variants, variant):
> > nodes.literal('', variants.tag_member.name),
> > nodes.Text(' is '),
> > nodes.literal('', '"%s"' % variant.name)]
> > - if variant.ifcond:
> > + if variant.ifcond.ifcond:
> > term.extend(self._nodes_for_ifcond(variant.ifcond))
> > return term
> >
> > @@ -209,7 +209,7 @@ def _nodes_for_enum_values(self, doc):
> > dlnode = nodes.definition_list()
> > for section in doc.args.values():
> > termtext = [nodes.literal('', section.member.name)]
> > - if section.member.ifcond:
> > + if section.member.ifcond.ifcond:
> >
> termtext.extend(self._nodes_for_ifcond(section.member.ifcond))
> > # TODO drop fallbacks when undocumented members are outlawed
> > if section.text:
> > @@ -277,7 +277,7 @@ def _nodes_for_sections(self, doc):
> > def _nodes_for_if_section(self, ifcond):
> > """Return list of doctree nodes for the "If" section"""
> > nodelist = []
> > - if ifcond:
> > + if ifcond.ifcond:
> > snode = self._make_section('If')
> > snode += nodes.paragraph(
> > '', '', *self._nodes_for_ifcond(ifcond, with_if=False)
> > diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
> > index 0e13d51054..3654825968 100644
> > --- a/scripts/qapi/commands.py
> > +++ b/scripts/qapi/commands.py
> > @@ -17,7 +17,6 @@
> > Dict,
> > List,
> > Optional,
> > - Sequence,
> > Set,
> > )
> >
> > @@ -31,6 +30,7 @@
> > from .schema import (
> > QAPISchema,
> > QAPISchemaFeature,
> > + QAPISchemaIfCond,
> > QAPISchemaObjectType,
> > QAPISchemaType,
> > )
> > @@ -301,7 +301,7 @@ def visit_end(self) -> None:
> > def visit_command(self,
> > name: str,
> > info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > arg_type: Optional[QAPISchemaObjectType],
> > ret_type: Optional[QAPISchemaType],
>
> Mechanical pattern #2: Sequence[str] becomes QAPISchemaIfCond. Also
> obvious import adjustments.
>
> > diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
> > index fee8c671e7..82475e84ec 100644
> > --- a/scripts/qapi/events.py
> > +++ b/scripts/qapi/events.py
> > @@ -12,7 +12,7 @@
> > See the COPYING file in the top-level directory.
> > """
> >
> > -from typing import List, Optional, Sequence
> > +from typing import List, Optional
> >
> > from .common import c_enum_const, c_name, mcgen
> > from .gen import QAPISchemaModularCVisitor, build_params, ifcontext
> > @@ -20,6 +20,7 @@
> > QAPISchema,
> > QAPISchemaEnumMember,
> > QAPISchemaFeature,
> > + QAPISchemaIfCond,
> > QAPISchemaObjectType,
> > )
> > from .source import QAPISourceInfo
> > @@ -227,7 +228,7 @@ def visit_end(self) -> None:
> > def visit_event(self,
> > name: str,
> > info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > arg_type: Optional[QAPISchemaObjectType],
> > boxed: bool) -> None:
> > diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
> > index 1fa503bdbd..1c5b190276 100644
> > --- a/scripts/qapi/gen.py
> > +++ b/scripts/qapi/gen.py
> > @@ -18,7 +18,6 @@
> > Dict,
> > Iterator,
> > Optional,
> > - Sequence,
> > Tuple,
> > )
> >
> > @@ -32,6 +31,7 @@
> > mcgen,
> > )
> > from .schema import (
> > + QAPISchemaIfCond,
> > QAPISchemaModule,
> > QAPISchemaObjectType,
> > QAPISchemaVisitor,
> > @@ -85,7 +85,7 @@ def write(self, output_dir: str) -> None:
> > fp.write(text)
> >
> >
> > -def _wrap_ifcond(ifcond: Sequence[str], before: str, after: str) -> str:
> > +def _wrap_ifcond(ifcond: QAPISchemaIfCond, before: str, after: str) ->
> str:
> > if before == after:
> > return after # suppress empty #if ... #endif
> >
> > @@ -95,9 +95,9 @@ def _wrap_ifcond(ifcond: Sequence[str], before: str,
> after: str) -> str:
> > if added[0] == '\n':
> > out += '\n'
> > added = added[1:]
> > - out += gen_if(ifcond)
> > + out += gen_if(ifcond.ifcond)
> > out += added
> > - out += gen_endif(ifcond)
> > + out += gen_endif(ifcond.ifcond)
> > return out
> >
> >
> > @@ -127,9 +127,9 @@ def build_params(arg_type:
> Optional[QAPISchemaObjectType],
> > class QAPIGenCCode(QAPIGen):
> > def __init__(self, fname: str):
> > super().__init__(fname)
> > - self._start_if: Optional[Tuple[Sequence[str], str, str]] = None
> > + self._start_if: Optional[Tuple[QAPISchemaIfCond, str, str]] =
> None
> >
> > - def start_if(self, ifcond: Sequence[str]) -> None:
> > + def start_if(self, ifcond: QAPISchemaIfCond) -> None:
> > assert self._start_if is None
> > self._start_if = (ifcond, self._body, self._preamble)
> >
> > @@ -187,7 +187,7 @@ def _bottom(self) -> str:
> >
> >
> > @contextmanager
> > -def ifcontext(ifcond: Sequence[str], *args: QAPIGenCCode) ->
> Iterator[None]:
> > +def ifcontext(ifcond: QAPISchemaIfCond, *args: QAPIGenCCode) ->
> Iterator[None]:
> > """
> > A with-statement context manager that wraps with `start_if()` /
> `end_if()`.
> >
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index 9a348ca2e5..77a8c33ad4 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -15,11 +15,9 @@
> > Any,
> > Dict,
> > Generic,
> > - Iterable,
> > List,
> > Optional,
> > Sequence,
> > - Tuple,
> > TypeVar,
> > Union,
> > )
> > @@ -38,6 +36,7 @@
> > QAPISchemaEntity,
> > QAPISchemaEnumMember,
> > QAPISchemaFeature,
> > + QAPISchemaIfCond,
> > QAPISchemaObjectType,
> > QAPISchemaObjectTypeMember,
> > QAPISchemaType,
> > @@ -91,11 +90,11 @@ class Annotated(Generic[_ValueT]):
> > """
> > # TODO: Remove after Python 3.7 adds @dataclass:
> > # pylint: disable=too-few-public-methods
> > - def __init__(self, value: _ValueT, ifcond: Iterable[str],
> > + def __init__(self, value: _ValueT, ifcond: QAPISchemaIfCond,
>
> Here, we have Iterable[str] instead of Sequence[str] before the patch.
> I figure use of Iterable was an attempt to be more general. Minor
> variation of pattern #2.
>
> > comment: Optional[str] = None):
> > self.value = value
> > self.comment: Optional[str] = comment
> > - self.ifcond: Tuple[str, ...] = tuple(ifcond)
> > + self.ifcond = ifcond
>
> Hmm. Here we change from Tuple, not from Sequence.
>
> I believe the next hunk has its only uses:
>
> >
> >
> > def _tree_to_qlit(obj: JSONValue,
> > @@ -125,10 +124,10 @@ def indent(level: int) -> str:
> > if obj.comment:
> > ret += indent(level) + f"/* {obj.comment} */\n"
> > if obj.ifcond:
> > - ret += gen_if(obj.ifcond)
> > + ret += gen_if(obj.ifcond.ifcond)
> > ret += _tree_to_qlit(obj.value, level)
> > if obj.ifcond:
> > - ret += '\n' + gen_endif(obj.ifcond)
> > + ret += '\n' + gen_endif(obj.ifcond.ifcond)
> > return ret
> >
> > ret = ''
>
> You update obj.ifcond to obj.ifcond.ifcond when used as argument of
> gen_if() and gen_endif(). This changes the argument from Tuple to
> Sequence. Fine, because Tuple is a special Sequence.
>
> Digression: I don't (anymore) understand why we made self.ifcond Tuple.
> John, do you remember?
>
> You don't update obj.ifcond when used as conditional. The code now
> calls gen_if() and gen_endif() even for an empty Sequence.
>
> I believe this can't actually happen because check_if() rejects [] with
> "'if' condition [] of %s is useless".
>
> Still, the mechanical change should update to obj.ifcond even when used
> as conditional.
>
> Are there other, possibly not so harmless uses of values that change
> from Sequence to QAPISchemaIfCond the patch doesn't update?
>
> Or asked differently: how did you find what to update?
>
Eh, you are asking me for something I spent just a few hours a few times
over the last year. Sorry!
Most probably simply with code reading/grepping, linter and the test suite.
>
> > @@ -254,7 +253,7 @@ def _gen_features(features:
> Sequence[QAPISchemaFeature]
> > return [Annotated(f.name, f.ifcond) for f in features]
> >
> > def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
> > - ifcond: Sequence[str] = (),
> > + ifcond: QAPISchemaIfCond = QAPISchemaIfCond(),
>
> Mechanical pattern #3: wrap QAPISchemaIfCond around the Sequence.
>
> For readability, you simplify the pure wrap QAPISchemaIfCond(()) to just
> QAPISchemaIfCond(), relying on QAPISchemaIfCond.__init__()'s default.
>
> However, it's also a subtle change from () to []. I believe that's
> okay, because we both are empty Sequences. Might be worth a mention in
> the commit message.
>
> > features: Sequence[QAPISchemaFeature] = ()) -> None:
> > """
> > Build and append a SchemaInfo object to self._trees.
> > @@ -305,7 +304,7 @@ def visit_builtin_type(self, name: str, info:
> Optional[QAPISourceInfo],
> > self._gen_tree(name, 'builtin', {'json-type': json_type})
> >
> > def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > members: List[QAPISchemaEnumMember],
> > prefix: Optional[str]) -> None:
> > @@ -316,14 +315,14 @@ def visit_enum_type(self, name: str, info:
> Optional[QAPISourceInfo],
> > )
> >
> > def visit_array_type(self, name: str, info:
> Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > element_type: QAPISchemaType) -> None:
> > element = self._use_type(element_type)
> > self._gen_tree('[' + element + ']', 'array', {'element-type':
> element},
> > ifcond)
> >
> > def visit_object_type_flat(self, name: str, info:
> Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > members:
> List[QAPISchemaObjectTypeMember],
> > variants: Optional[QAPISchemaVariants])
> -> None:
> > @@ -336,7 +335,7 @@ def visit_object_type_flat(self, name: str, info:
> Optional[QAPISourceInfo],
> > self._gen_tree(name, 'object', obj, ifcond, features)
> >
> > def visit_alternate_type(self, name: str, info:
> Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > variants: QAPISchemaVariants) -> None:
> > self._gen_tree(
> > @@ -348,7 +347,7 @@ def visit_alternate_type(self, name: str, info:
> Optional[QAPISourceInfo],
> > )
> >
> > def visit_command(self, name: str, info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > arg_type: Optional[QAPISchemaObjectType],
> > ret_type: Optional[QAPISchemaType], gen: bool,
> > @@ -367,7 +366,8 @@ def visit_command(self, name: str, info:
> Optional[QAPISourceInfo],
> > self._gen_tree(name, 'command', obj, ifcond, features)
> >
> > def visit_event(self, name: str, info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str], features:
> List[QAPISchemaFeature],
> > + ifcond: QAPISchemaIfCond,
> > + features: List[QAPISchemaFeature],
> > arg_type: Optional[QAPISchemaObjectType],
> > boxed: bool) -> None:
> > assert self._schema is not None
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index d1d27ff7ee..5e44164bd1 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -25,6 +25,11 @@
> > from .parser import QAPISchemaParser
> >
> >
> > +class QAPISchemaIfCond:
> > + def __init__(self, ifcond=None):
> > + self.ifcond = ifcond or []
> > +
> > +
> > class QAPISchemaEntity:
> > meta: Optional[str] = None
> >
> > @@ -42,7 +47,7 @@ def __init__(self, name: str, info, doc, ifcond=None,
> features=None):
> > # such place).
> > self.info = info
> > self.doc = doc
> > - self._ifcond = ifcond or []
> > + self._ifcond = ifcond or QAPISchemaIfCond()
>
> This is an instance of mechanical pattern #3 where we wrap the value
> without changing it.
>
> Before the patch we use both () and [] as "no conditions". After the
> patch, we always use [], unless something passes another empty sequence
> to QAPISchemaIfCond.__init__(), which I don't think is the case.
> Observation, not a request.
>
>
> > self.features = features or []
> > self._checked = False
> >
> > @@ -78,6 +83,7 @@ def set_module(self, schema):
> > @property
> > def ifcond(self):
> > assert self._checked
> > + assert isinstance(self._ifcond, QAPISchemaIfCond)
> > return self._ifcond
>
> Non-mechanical hunk. The commit message claims to list them ("Except
> for ..., it's a mechanical change"), but it doesn't. Easy enough to
> fix.
>
dropped
> >
> > def is_implicit(self):
> > @@ -593,7 +599,7 @@ def check(self, schema, seen):
> > self.info,
> > "discriminator member '%s' of %s must not be
> optional"
> > % (self._tag_name, base))
> > - if self.tag_member.ifcond:
> > + if self.tag_member.ifcond.ifcond:
> > raise QAPISemError(
> > self.info,
> > "discriminator member '%s' of %s must not be
> conditional"
> > @@ -601,7 +607,7 @@ def check(self, schema, seen):
> > else: # simple union
> > assert isinstance(self.tag_member.type, QAPISchemaEnumType)
> > assert not self.tag_member.optional
> > - assert self.tag_member.ifcond == []
> > + assert self.tag_member.ifcond.ifcond == []
> > if self._tag_name: # flat union
> > # branches that are not explicitly covered get an empty type
> > cases = {v.name for v in self.variants}
> > @@ -646,7 +652,7 @@ def __init__(self, name, info, ifcond=None):
> > assert isinstance(name, str)
> > self.name = name
> > self.info = info
> > - self.ifcond = ifcond or []
> > + self.ifcond = ifcond or QAPISchemaIfCond()
> > self.defined_in = None
> >
> > def set_defined_in(self, name):
> > @@ -968,11 +974,13 @@ def _def_predefineds(self):
> > def _make_features(self, features, info):
> > if features is None:
> > return []
> > - return [QAPISchemaFeature(f['name'], info, f.get('if'))
> > + return [QAPISchemaFeature(f['name'], info,
> > + QAPISchemaIfCond(f.get('if')))
> > for f in features]
> >
> > def _make_enum_members(self, values, info):
> > - return [QAPISchemaEnumMember(v['name'], info, v.get('if'))
> > + return [QAPISchemaEnumMember(v['name'], info,
> > + QAPISchemaIfCond(v.get('if')))
> > for v in values]
> >
>
> Two more instances of pattern #3, only here we wrap values we get from
> the JSON parser. These are either None or non-empty lists.
>
> > def _make_implicit_enum_type(self, name, info, ifcond, values):
> > @@ -1008,7 +1016,10 @@ def _make_implicit_object_type(self, name, info,
> ifcond, role, members):
> 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
> >
> > # pylint: disable=protected-access
> > - assert (ifcond or []) == typ._ifcond
> > + if isinstance(ifcond, QAPISchemaIfCond):
> > + assert ifcond.ifcond == typ._ifcond.ifcond
> > + else:
> > + assert ifcond == typ._ifcond
> > else:
> > self._def_entity(QAPISchemaObjectType(
> > name, info, None, ifcond, None, None, members, None))
>
> This is the non-mechanical change mentioned in the commit message.
>
> Can you explain where the two cases come from?
>
>
_make_simple_variant() calls _make_implicit_object_type() with
self.lookup_type(typ).
I think it could instead call with the ._ifcond value.
To be done after?
> > @@ -1018,7 +1029,7 @@ def _def_enum_type(self, expr, info, doc):
> > name = expr['enum']
> > data = expr['data']
> > prefix = expr.get('prefix')
> > - ifcond = expr.get('if')
> > + ifcond = QAPISchemaIfCond(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > self._def_entity(QAPISchemaEnumType(
> > name, info, doc, ifcond, features,
> > @@ -1036,7 +1047,8 @@ def _make_member(self, name, typ, ifcond,
> features, info):
> > self._make_features(features,
> info))
> >
> > def _make_members(self, data, info):
> > - return [self._make_member(key, value['type'], value.get('if'),
> > + return [self._make_member(key, value['type'],
> > + QAPISchemaIfCond(value.get('if')),
> > value.get('features'), info)
> > for (key, value) in data.items()]
> >
> > @@ -1044,7 +1056,7 @@ def _def_struct_type(self, expr, info, doc):
> > name = expr['struct']
> > base = expr.get('base')
> > data = expr['data']
> > - ifcond = expr.get('if')
> > + ifcond = QAPISchemaIfCond(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > self._def_entity(QAPISchemaObjectType(
> > name, info, doc, ifcond, features, base,
> > @@ -1067,7 +1079,7 @@ def _def_union_type(self, expr, info, doc):
> > name = expr['union']
> > data = expr['data']
> > base = expr.get('base')
> > - ifcond = expr.get('if')
> > + ifcond = QAPISchemaIfCond(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > tag_name = expr.get('discriminator')
> > tag_member = None
> > @@ -1076,15 +1088,19 @@ def _def_union_type(self, expr, info, doc):
> > name, info, ifcond,
> > 'base', self._make_members(base, info))
> > if tag_name:
> > - variants = [self._make_variant(key, value['type'],
> > - value.get('if'), info)
> > - for (key, value) in data.items()]
> > + variants = [
> > + self._make_variant(key, value['type'],
> > + QAPISchemaIfCond(value.get('if')),
> > + info)
> > + for (key, value) in data.items()]
> > members = []
> > else:
> > - variants = [self._make_simple_variant(key, value['type'],
> > - value.get('if'), info)
> > - for (key, value) in data.items()]
> > - enum = [{'name': v.name, 'if': v.ifcond} for v in variants]
> > + variants = [
> > + self._make_simple_variant(key, value['type'],
> > +
> QAPISchemaIfCond(value.get('if')),
> > + info)
> > + for (key, value) in data.items()]
> > + enum = [{'name': v.name, 'if': v.ifcond.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]
> > @@ -1097,11 +1113,13 @@ def _def_union_type(self, expr, info, doc):
> > def _def_alternate_type(self, expr, info, doc):
> > name = expr['alternate']
> > data = expr['data']
> > - ifcond = expr.get('if')
> > + ifcond = QAPISchemaIfCond(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > - variants = [self._make_variant(key, value['type'],
> value.get('if'),
> > - info)
> > - for (key, value) in data.items()]
> > + variants = [
> > + self._make_variant(key, value['type'],
> > + QAPISchemaIfCond(value.get('if')),
> > + info)
> > + for (key, value) in data.items()]
> > tag_member = QAPISchemaObjectTypeMember('type', info, 'QType',
> False)
> > self._def_entity(
> > QAPISchemaAlternateType(name, info, doc, ifcond, features,
> > @@ -1118,7 +1136,7 @@ def _def_command(self, expr, info, doc):
> > allow_oob = expr.get('allow-oob', False)
> > allow_preconfig = expr.get('allow-preconfig', False)
> > coroutine = expr.get('coroutine', False)
> > - ifcond = expr.get('if')
> > + ifcond = QAPISchemaIfCond(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > if isinstance(data, OrderedDict):
> > data = self._make_implicit_object_type(
> > @@ -1137,7 +1155,7 @@ def _def_event(self, expr, info, doc):
> > name = expr['event']
> > data = expr.get('data')
> > boxed = expr.get('boxed', False)
> > - ifcond = expr.get('if')
> > + ifcond = QAPISchemaIfCond(expr.get('if'))
> > features = self._make_features(expr.get('features'), info)
> > if isinstance(data, OrderedDict):
> > data = self._make_implicit_object_type(
> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> > index 20d572a23a..3673cf0f49 100644
> > --- a/scripts/qapi/types.py
> > +++ b/scripts/qapi/types.py
> > @@ -13,7 +13,7 @@
> > # See the COPYING file in the top-level directory.
> > """
> >
> > -from typing import List, Optional, Sequence
> > +from typing import List, Optional
> >
> > from .common import (
> > c_enum_const,
> > @@ -27,6 +27,7 @@
> > QAPISchema,
> > QAPISchemaEnumMember,
> > QAPISchemaFeature,
> > + QAPISchemaIfCond,
> > QAPISchemaObjectType,
> > QAPISchemaObjectTypeMember,
> > QAPISchemaType,
> > @@ -50,13 +51,13 @@ def gen_enum_lookup(name: str,
> > ''',
> > c_name=c_name(name))
> > for memb in members:
> > - ret += gen_if(memb.ifcond)
> > + ret += gen_if(memb.ifcond.ifcond)
> > index = c_enum_const(name, memb.name, prefix)
> > ret += mcgen('''
> > [%(index)s] = "%(name)s",
> > ''',
> > index=index, name=memb.name)
> > - ret += gen_endif(memb.ifcond)
> > + ret += gen_endif(memb.ifcond.ifcond)
> >
> > ret += mcgen('''
> > },
> > @@ -80,12 +81,12 @@ def gen_enum(name: str,
> > c_name=c_name(name))
> >
> > for memb in enum_members:
> > - ret += gen_if(memb.ifcond)
> > + ret += gen_if(memb.ifcond.ifcond)
> > ret += mcgen('''
> > %(c_enum)s,
> > ''',
> > c_enum=c_enum_const(name, memb.name, prefix))
> > - ret += gen_endif(memb.ifcond)
> > + ret += gen_endif(memb.ifcond.ifcond)
> >
> > ret += mcgen('''
> > } %(c_name)s;
> > @@ -125,7 +126,7 @@ def gen_array(name: str, element_type:
> QAPISchemaType) -> str:
> > def gen_struct_members(members: List[QAPISchemaObjectTypeMember]) ->
> str:
> > ret = ''
> > for memb in members:
> > - ret += gen_if(memb.ifcond)
> > + ret += gen_if(memb.ifcond.ifcond)
> > if memb.optional:
> > ret += mcgen('''
> > bool has_%(c_name)s;
> > @@ -135,11 +136,11 @@ def gen_struct_members(members:
> List[QAPISchemaObjectTypeMember]) -> str:
> > %(c_type)s %(c_name)s;
> > ''',
> > c_type=memb.type.c_type(), c_name=c_name(memb.name
> ))
> > - ret += gen_endif(memb.ifcond)
> > + ret += gen_endif(memb.ifcond.ifcond)
> > return ret
> >
> >
> > -def gen_object(name: str, ifcond: Sequence[str],
> > +def gen_object(name: str, ifcond: QAPISchemaIfCond,
> > base: Optional[QAPISchemaObjectType],
> > members: List[QAPISchemaObjectTypeMember],
> > variants: Optional[QAPISchemaVariants]) -> str:
> > @@ -158,7 +159,7 @@ def gen_object(name: str, ifcond: Sequence[str],
> > ret += mcgen('''
> >
> > ''')
> > - ret += gen_if(ifcond)
> > + ret += gen_if(ifcond.ifcond)
> > ret += mcgen('''
> > struct %(c_name)s {
> > ''',
> > @@ -192,7 +193,7 @@ def gen_object(name: str, ifcond: Sequence[str],
> > ret += mcgen('''
> > };
> > ''')
> > - ret += gen_endif(ifcond)
> > + ret += gen_endif(ifcond.ifcond)
> >
> > return ret
> >
> > @@ -219,13 +220,13 @@ def gen_variants(variants: QAPISchemaVariants) ->
> str:
> > for var in variants.variants:
> > if var.type.name == 'q_empty':
> > continue
> > - ret += gen_if(var.ifcond)
> > + ret += gen_if(var.ifcond.ifcond)
> > ret += mcgen('''
> > %(c_type)s %(c_name)s;
> > ''',
> > c_type=var.type.c_unboxed_type(),
> > c_name=c_name(var.name))
> > - ret += gen_endif(var.ifcond)
> > + ret += gen_endif(var.ifcond.ifcond)
> >
> > ret += mcgen('''
> > } u;
> > @@ -307,7 +308,7 @@ def _gen_type_cleanup(self, name: str) -> None:
> > def visit_enum_type(self,
> > name: str,
> > info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > members: List[QAPISchemaEnumMember],
> > prefix: Optional[str]) -> None:
> > @@ -318,7 +319,7 @@ def visit_enum_type(self,
> > def visit_array_type(self,
> > name: str,
> > info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > element_type: QAPISchemaType) -> None:
> > with ifcontext(ifcond, self._genh, self._genc):
> > self._genh.preamble_add(gen_fwd_object_or_array(name))
> > @@ -328,7 +329,7 @@ def visit_array_type(self,
> > def visit_object_type(self,
> > name: str,
> > info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > base: Optional[QAPISchemaObjectType],
> > members: List[QAPISchemaObjectTypeMember],
> > @@ -351,7 +352,7 @@ def visit_object_type(self,
> > def visit_alternate_type(self,
> > name: str,
> > info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > variants: QAPISchemaVariants) -> None:
> > with ifcontext(ifcond, self._genh):
> > diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
> > index 9e96f3c566..67721b2470 100644
> > --- a/scripts/qapi/visit.py
> > +++ b/scripts/qapi/visit.py
> > @@ -13,7 +13,7 @@
> > See the COPYING file in the top-level directory.
> > """
> >
> > -from typing import List, Optional, Sequence
> > +from typing import List, Optional
> >
> > from .common import (
> > c_enum_const,
> > @@ -29,6 +29,7 @@
> > QAPISchemaEnumMember,
> > QAPISchemaEnumType,
> > QAPISchemaFeature,
> > + QAPISchemaIfCond,
> > QAPISchemaObjectType,
> > QAPISchemaObjectTypeMember,
> > QAPISchemaType,
> > @@ -78,7 +79,7 @@ def gen_visit_object_members(name: str,
> >
> > for memb in members:
> > deprecated = 'deprecated' in [f.name for f in memb.features]
> > - ret += gen_if(memb.ifcond)
> > + ret += gen_if(memb.ifcond.ifcond)
> > if memb.optional:
> > ret += mcgen('''
> > if (visit_optional(v, "%(name)s", &obj->has_%(c_name)s)) {
> > @@ -111,7 +112,7 @@ def gen_visit_object_members(name: str,
> > ret += mcgen('''
> > }
> > ''')
> > - ret += gen_endif(memb.ifcond)
> > + ret += gen_endif(memb.ifcond.ifcond)
> >
> > if variants:
> > tag_member = variants.tag_member
> > @@ -125,7 +126,7 @@ def gen_visit_object_members(name: str,
> > for var in variants.variants:
> > case_str = c_enum_const(tag_member.type.name, var.name,
> > tag_member.type.prefix)
> > - ret += gen_if(var.ifcond)
> > + ret += gen_if(var.ifcond.ifcond)
> > if var.type.name == 'q_empty':
> > # valid variant and nothing to do
> > ret += mcgen('''
> > @@ -141,7 +142,7 @@ def gen_visit_object_members(name: str,
> > case=case_str,
> > c_type=var.type.c_name(), c_name=c_name(
> var.name))
> >
> > - ret += gen_endif(var.ifcond)
> > + ret += gen_endif(var.ifcond.ifcond)
> > ret += mcgen('''
> > default:
> > abort();
> > @@ -227,7 +228,7 @@ def gen_visit_alternate(name: str, variants:
> QAPISchemaVariants) -> str:
> > c_name=c_name(name))
> >
> > for var in variants.variants:
> > - ret += gen_if(var.ifcond)
> > + ret += gen_if(var.ifcond.ifcond)
> > ret += mcgen('''
> > case %(case)s:
> > ''',
> > @@ -253,7 +254,7 @@ def gen_visit_alternate(name: str, variants:
> QAPISchemaVariants) -> str:
> > ret += mcgen('''
> > break;
> > ''')
> > - ret += gen_endif(var.ifcond)
> > + ret += gen_endif(var.ifcond.ifcond)
> >
> > ret += mcgen('''
> > case QTYPE_NONE:
> > @@ -352,7 +353,7 @@ def _begin_user_module(self, name: str) -> None:
> > def visit_enum_type(self,
> > name: str,
> > info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > members: List[QAPISchemaEnumMember],
> > prefix: Optional[str]) -> None:
> > @@ -363,7 +364,7 @@ def visit_enum_type(self,
> > def visit_array_type(self,
> > name: str,
> > info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > element_type: QAPISchemaType) -> None:
> > with ifcontext(ifcond, self._genh, self._genc):
> > self._genh.add(gen_visit_decl(name))
> > @@ -372,7 +373,7 @@ def visit_array_type(self,
> > def visit_object_type(self,
> > name: str,
> > info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > base: Optional[QAPISchemaObjectType],
> > members: List[QAPISchemaObjectTypeMember],
> > @@ -394,7 +395,7 @@ def visit_object_type(self,
> > def visit_alternate_type(self,
> > name: str,
> > info: Optional[QAPISourceInfo],
> > - ifcond: Sequence[str],
> > + ifcond: QAPISchemaIfCond,
> > features: List[QAPISchemaFeature],
> > variants: QAPISchemaVariants) -> None:
> > with ifcontext(ifcond, self._genh, self._genc):
> > diff --git a/tests/qapi-schema/test-qapi.py
> b/tests/qapi-schema/test-qapi.py
> > index f1c4deb9a5..7907b4ac3a 100755
> > --- a/tests/qapi-schema/test-qapi.py
> > +++ b/tests/qapi-schema/test-qapi.py
> > @@ -94,8 +94,8 @@ def _print_variants(variants):
> >
> > @staticmethod
> > def _print_if(ifcond, indent=4):
> > - if ifcond:
> > - print('%sif %s' % (' ' * indent, ifcond))
> > + if ifcond.ifcond:
> > + print('%sif %s' % (' ' * indent, ifcond.ifcond))
> >
> > @classmethod
> > def _print_features(cls, features, indent=4):
>
>
