Marc-André Lureau <marcandre.lur...@gmail.com> writes:
> Hi
>
> On Mon, Jun 14, 2021 at 4:20 PM Markus Armbruster <arm...@redhat.com> wrote:
>
>> marcandre.lur...@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lur...@redhat.com>
>> >
>> > Wrap the 'if' condition in a higher-level object. Not only does this
>>
>> I can see "wrap in an object". I'm afraid don't get what makes it
>> "higher-level".
>>
>
> ok
>
>
>> > provide more type safety but it also enables further refactoring without
>> > too much churn.
>>
>> I figure by "more type safety" you mean "can't accidentally confuse some
>> other list of strings with a conditional", which is true, but isn't
>> really about *type* safety.
>>
>> Maybe:
>>
>> Wrap the 'if' condition in an object. Not only is this a bit safer,
>> it also enables further refactoring without too much churn.
>>
>>
> ok
>
>> > The following patches will change the syntax of the schema 'if'
>> > conditions to be predicate expressions, and will generate code for
>> > different target languages (C, and Rust in another series).
>>
>> Since different target languages aren't actually generated in this
>> series, I'd say "and will enable generating code for different target
>> languages, such as Rust."
>>
>
> ok
>
>
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
>> > Reviewed-by: Stefan Hajnoczi <stefa...@redhat.com>
>> > Tested-by: John Snow <js...@redhat.com>
>> > ---
>> > docs/sphinx/qapidoc.py | 2 +-
>> > scripts/qapi/commands.py | 4 +-
>> > scripts/qapi/events.py | 5 ++-
>> > scripts/qapi/gen.py | 14 +++----
>> > scripts/qapi/introspect.py | 26 ++++++------
>> > scripts/qapi/schema.py | 74 +++++++++++++++++++++++-----------
>> > scripts/qapi/types.py | 33 +++++++--------
>> > scripts/qapi/visit.py | 23 ++++++-----
>> > tests/qapi-schema/test-qapi.py | 2 +-
>> > 9 files changed, 106 insertions(+), 77 deletions(-)
>> >
>> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
>> > index 87c67ab23f..b737949007 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],
>> > nodes.Text(', '))
>> > if not with_if:
>> > return condlist
>>
>> Note for later: many hunks replace ifcond (the unwrapped Sequence[str])
>> by ifcond.ifcond (the wrapped one, with the wrapper peeled off).
>> Mechanical.
>>
>> > 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],
>>
>> Note for later: many hunks replace ifcond: Sequence[str] or
>> Iterable[str] by ifcond: QAPISchemaIfCond. Mechanical.
>>
>> > 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,
>> > comment: Optional[str] = None):
>> > self.value = value
>> > self.comment: Optional[str] = comment
>> > - self.ifcond: Tuple[str, ...] = tuple(ifcond)
>> > + self.ifcond = ifcond
>> >
>> >
>> > 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 = ''
>> > @@ -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(),
>>
>> The same QAPISchemaIfCond object gets reused every time we don't pass an
>> @ifcond argument. Looks a bit scary, but works, because we don't ever
>> mutate it.
>>
>> Elsewhere, we None as default, though: QAPISchemaEntity.__init__(),
>> QAPISchemaMember.__init__().
>>
>
> A mechanical change, isn't it?
We do the same thing in two different ways.
One: default parameter ifcond to QAPISchemaIfCond(), done.
Two: default it to None, use it like ifcond or QAPISchemaIfCond().
The first way is less verbose, but looks like a Python rookie mistake at
a glance.
Can we agree on one way to do this?
>> > 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..bc357ebbfa 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
>> > @@ -25,6 +25,20 @@
>> > from .parser import QAPISchemaParser
>> >
>> >
>> > +class QAPISchemaIfCond:
>> > + def __init__(self, ifcond=None):
>> > + self.ifcond = ifcond or []
>> > +
>> > + # Returns true if the condition is not void
>> > + def __bool__(self):
>> > + return bool(self.ifcond)
>>
>> I'm not sure I like this one.
>>
>> In the two places where we default parameter icond=None, we use
>>
>> ifcond or QAPISchemaIfCond()
>>
>> to flip to the default value we actually want. Works as intended for
>> None and for non-empty QAPISchemaIfCond. For empty QAPISchemaIfCond, it
>> throws away the value and creates a new, equivalent one. Works, but
>> kind of by accident.
>>
>> This is an instance of a more general problem: when I see an
>> Optional[ObjectType], I expect it to be falsy if and only if it's None.
>> Perhaps I shouldn't. Doesn't mean we should press __bool__() into
>> service for checking "is there a condition". A boring non-dunder method
>> might be clearer.
>>
>> I understand what you mean by "condition is void", but it sounds a bit
>> odd to me. How do you like "Is a condition present?"
>>
>
> The current code uses falsy values for ifcond (whether it is [], (), None
> whatever). Implementing __bool__() allowed to keep the existing condition
> code (ie: if ifcond).
>
> After the wrapping object is introduced, we have "if ifcond.ifcond", which
> is quite ugly.
It is.
> If you think "if ifcond" isn't clear enough (with __bool__()), we can have
> "if ifcond.is_present()". I don't have a preference.
I think I'd prefer .is_present(). I'm kind of wary of overriding
dunders when it can make innocent-looking code behave in possibly
surprising ways. Perhaps I just haven't been steeped in the Python
sauce long enough.
John, what do you think?
>> > +
>> > + def __eq__(self, other):
>> > + if not isinstance(other, QAPISchemaIfCond):
>> > + return NotImplemented
>> > + return self.ifcond == other.ifcond
>>
>> Stupid question: why do we need to override __eq__()?
>>
>> Hmm, probably for _make_implicit_object_type().
>>
>>
> Yes, the code works with schema objects and ifcond. I'll special case the
> assertion for now, and remove that method.
>
>
>> Why raise on type mismatch? Feels rather un-pythonic to me.
>>
>
> What else should it do? Could probably be removed for now.
Python lets me compare arbitrary objects regardless of type. Different
objects are unqual, unless __eq__ makes them equal.
You want to make different QAPISchemaIfCond wrapping the equal .ifcond
equal. Makes sense to me. But I don't see a need for also changing the
case "@other is not a QAPISchemaIfCond" from False to NotImplemented.
>> > +
>> > +
>> > class QAPISchemaEntity:
>> > meta: Optional[str] = None
>> >
>> > @@ -42,7 +56,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()
>> > self.features = features or []
>> > self._checked = False
>> >
>> > @@ -77,7 +91,7 @@ def set_module(self, schema):
>> >
>> > @property
>> > def ifcond(self):
>> > - assert self._checked
>> > + assert self._checked and isinstance(self._ifcond,
>> > QAPISchemaIfCond)
>> > return self._ifcond
>> >
>> > def is_implicit(self):
>> > @@ -601,7 +615,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 not self.tag_member.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 +660,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 +982,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]
>> >
>>
>> Note for later: several hunks wrap condition expressions in an object
>> like this. Mechanical.
>>
>> > def _make_implicit_enum_type(self, name, info, ifcond, values):
>> > @@ -1008,7 +1024,7 @@ def _make_implicit_object_type(self, name, info,
>> > ifcond, role, members):
>> > # TODO kill simple unions or implement the disjunction
>> >
>> > # pylint: disable=protected-access
>> > - assert (ifcond or []) == typ._ifcond
>> > + assert ifcond == typ._ifcond
>>
>> I'm not sure I understand this part. Leaving for later.
>>
>> > else:
>> > self._def_entity(QAPISchemaObjectType(
>> > name, info, None, ifcond, None, None, members, None))
>> > @@ -1018,7 +1034,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 +1052,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 +1061,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 +1084,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 +1093,21 @@ 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()
>> > + ]
>>
>> I think we more usually put the closing parenthesis like this:
>>
>> variants = [
>> ...
>> for (key, value) in data.items()]
>>
>> More of the same below.
>>
>
> Works for me.
>
>
>> > 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 +1120,14 @@ 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 +1144,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 +1163,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..2ec328b22e 100755
>> > --- a/tests/qapi-schema/test-qapi.py
>> > +++ b/tests/qapi-schema/test-qapi.py
>> > @@ -95,7 +95,7 @@ def _print_variants(variants):
>> > @staticmethod
>> > def _print_if(ifcond, indent=4):
>> > if ifcond:
>> > - print('%sif %s' % (' ' * indent, ifcond))
>> > + print('%sif %s' % (' ' * indent, ifcond.ifcond))
>> >
>> > @classmethod
>> > def _print_features(cls, features, indent=4):
>>
>> If feel this is a bit harder to review than necessary, because you take
>> two steps at once:
>>
>> 1. Wrap Sequence[str] in an object
>>
>> 2. Add methods to the object to clean up the resulting mess some
>>
>> Step 1. by itself should be pretty much mechanical: adjust the type
>> hints, create the wrapper object on write, peel it off on read. The
>> result will be slightly ugly in places.
>>
>> I'd expect step 2 to be much smaller, and easier to understand. It
>> could perhaps be split into one patch per method, but I hope it's
>> reviewable just fine even without.
Hope you didn't miss this remark.
