[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 07/26] qapi: add 'if' condition on top-level sch
|
From: |
Marc-André Lureau |
|
Subject: |
Re: [Qemu-devel] [PATCH 07/26] qapi: add 'if' condition on top-level schema elements |
|
Date: |
Tue, 22 Aug 2017 13:17:55 +0200 |
Hi
On Wed, Aug 16, 2017 at 5:43 PM, Markus Armbruster <address@hidden> wrote:
> Marc-André Lureau <address@hidden> writes:
>
>> Add 'if' c-preprocessor condition on top-level schema elements:
>> struct, enum, union, alternate, command, event.
>
> An example would be useful here. Your cover letter has nice ones, would
> be a shame not to preserve them for posterity in the commit log.
>
ok
>> Variants objects types are created outside of #if blocks, since they
>
> What are "variants objects types"?
>
I think I meant implicit objects types, that may be created by variant.
>> may be shared by various types. Even if unused, this shouldn't be an
>> issue, since those are internal types.
>>
>> Note that there is no checks in qapi scripts to verify that elements
>
> there are
>
>> have compatible conditions (ex: if-struct used by a if-foo
>> command). This may thus fail at C build time if they don't share the
>> same subset of conditions.
>
> Fair enough.
>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>> scripts/qapi.py | 153
>> +++++++++++++++++++++++++-------
>> scripts/qapi-commands.py | 4 +-
>> scripts/qapi-event.py | 4 +-
>> scripts/qapi-introspect.py | 46 ++++++----
>> scripts/qapi-types.py | 51 +++++++----
>> scripts/qapi-visit.py | 13 ++-
>> scripts/qapi2texi.py | 10 +--
>> tests/Makefile.include | 1 +
>> tests/qapi-schema/bad-if.err | 1 +
>> tests/qapi-schema/bad-if.exit | 1 +
>> tests/qapi-schema/bad-if.json | 3 +
>> tests/qapi-schema/bad-if.out | 0
>> tests/qapi-schema/qapi-schema-test.json | 20 +++++
>> tests/qapi-schema/qapi-schema-test.out | 31 +++++++
>> tests/qapi-schema/test-qapi.py | 21 +++--
>> 15 files changed, 272 insertions(+), 87 deletions(-)
>> create mode 100644 tests/qapi-schema/bad-if.err
>> create mode 100644 tests/qapi-schema/bad-if.exit
>> create mode 100644 tests/qapi-schema/bad-if.json
>> create mode 100644 tests/qapi-schema/bad-if.out
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 4ecc19e944..79ba1e93da 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -639,6 +639,16 @@ def add_name(name, info, meta, implicit=False):
>> all_names[name] = meta
>>
>>
>> +def check_if(expr, info):
>> + ifcond = expr.get('if')
>> + if not ifcond or isinstance(ifcond, str):
>> + return
>> + if (not isinstance(ifcond, list) or
>> + any([not isinstance(elt, str) for elt in ifcond])):
>> + raise QAPISemError(info, "'if' condition requires a string or "
>> + "a list of string")
>
> Wait a second! What's this list business? The commit message doesn't
> say.
Updated commit message, and documented in docs/devel/qapi-code-gen.txt
>
> Also, pep8 gripes:
>
> scripts/qapi.py:647:9: E129 visually indented line with same indent as
> next logical line
>
fixed
>> +
>> +
>> def check_type(info, source, value, allow_array=False,
>> allow_dict=False, allow_optional=False,
>> allow_metas=[]):
>> @@ -865,6 +875,7 @@ def check_keys(expr_elem, meta, required, optional=[]):
>> expr = expr_elem['expr']
>> info = expr_elem['info']
>> name = expr[meta]
>> + optional.append('if')
>
> Caution!
>
> $ python
> Python 2.7.13 (default, May 10 2017, 20:04:36)
> >>> def surprise(arg=[]):
> ... arg.append('if')
> ... return arg
> ...
> >>> surprise()
> ['if']
> >>> surprise()
> ['if', 'if']
> >>> surprise()
> ['if', 'if', 'if']
>
> Never modify an argument that has list or dictionary default value. To
> avoid the temptation, never use such defaul values.
Oops! Without bad consequences here, but fixed anyway.
>
>> if not isinstance(name, str):
>> raise QAPISemError(info, "'%s' key must have a string value" % meta)
>> required = required + [meta]
>> @@ -880,6 +891,8 @@ def check_keys(expr_elem, meta, required, optional=[]):
>> raise QAPISemError(info,
>> "'%s' of %s '%s' should only use true value"
>> % (key, meta, name))
>> + if key == 'if':
>> + check_if(expr, info)
>> for key in required:
>> if key not in expr:
>> raise QAPISemError(info, "Key '%s' is missing from %s '%s'"
>> @@ -989,6 +1002,10 @@ class QAPISchemaEntity(object):
>> # such place).
>> self.info = info
>> self.doc = doc
>> + self.ifcond = None
>> +
>> + def set_ifcond(self, ifcond):
>> + self.ifcond = ifcond
>
> @ifcond is an awkward name, but I don't have better ideas.
Yeah, I got used to it now ;)
>
>>
>> def c_name(self):
>> return c_name(self.name)
>> @@ -1017,26 +1034,26 @@ class QAPISchemaVisitor(object):
>> def visit_builtin_type(self, name, info, json_type):
>> pass
>>
>> - def visit_enum_type(self, name, info, values, prefix):
>> + def visit_enum_type(self, name, info, values, prefix, ifcond):
>> pass
>>
>> - def visit_array_type(self, name, info, element_type):
>> + def visit_array_type(self, name, info, element_type, ifcond):
>> pass
>>
>> - def visit_object_type(self, name, info, base, members, variants):
>> + def visit_object_type(self, name, info, base, members, variants,
>> ifcond):
>> pass
>>
>> - def visit_object_type_flat(self, name, info, members, variants):
>> + def visit_object_type_flat(self, name, info, members, variants, ifcond):
>> pass
>>
>> - def visit_alternate_type(self, name, info, variants):
>> + def visit_alternate_type(self, name, info, variants, ifcond):
>> pass
>>
>> def visit_command(self, name, info, arg_type, ret_type,
>> - gen, success_response, boxed):
>> + gen, success_response, boxed, ifcond):
>> pass
>>
>> - def visit_event(self, name, info, arg_type, boxed):
>> + def visit_event(self, name, info, arg_type, boxed, ifcond):
>> pass
>>
>>
>> @@ -1136,7 +1153,7 @@ class QAPISchemaEnumType(QAPISchemaType):
>>
>> def visit(self, visitor):
>> visitor.visit_enum_type(self.name, self.info,
>> - self.member_names(), self.prefix)
>> + self.member_names(), self.prefix,
>> self.ifcond)
>>
>>
>> class QAPISchemaArrayType(QAPISchemaType):
>> @@ -1149,6 +1166,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
>>
>> def is_implicit(self):
>> return True
>> @@ -1166,7 +1184,8 @@ class QAPISchemaArrayType(QAPISchemaType):
>> return 'array of ' + elt_doc_type
>>
>> def visit(self, visitor):
>> - visitor.visit_array_type(self.name, self.info, self.element_type)
>> + visitor.visit_array_type(self.name, self.info, self.element_type,
>> + self.ifcond)
>>
>>
>> class QAPISchemaObjectType(QAPISchemaType):
>> @@ -1247,9 +1266,10 @@ class QAPISchemaObjectType(QAPISchemaType):
>>
>> def visit(self, visitor):
>> visitor.visit_object_type(self.name, self.info,
>> - self.base, self.local_members,
>> self.variants)
>> + self.base, self.local_members,
>> self.variants,
>> + self.ifcond)
>> visitor.visit_object_type_flat(self.name, self.info,
>> - self.members, self.variants)
>> + self.members, self.variants,
>> self.ifcond)
>
> You break the line before self.ifcond almost everywhere, and when you
> don't, the line gets long. This one goes over the limit:
>
> scripts/qapi.py:1285:80: E501 line too long (80 > 79 characters)
>
> Suggest to break it before self.ifcond everywhere.
>
ok
>>
>>
>> class QAPISchemaMember(object):
>> @@ -1392,7 +1412,8 @@ class QAPISchemaAlternateType(QAPISchemaType):
>> return 'value'
>>
>> def visit(self, visitor):
>> - visitor.visit_alternate_type(self.name, self.info, self.variants)
>> + visitor.visit_alternate_type(self.name, self.info,
>> + self.variants, self.ifcond)
>>
>> def is_empty(self):
>> return False
>> @@ -1434,7 +1455,8 @@ class QAPISchemaCommand(QAPISchemaEntity):
>> def visit(self, visitor):
>> visitor.visit_command(self.name, self.info,
>> self.arg_type, self.ret_type,
>> - self.gen, self.success_response, self.boxed)
>> + self.gen, self.success_response, self.boxed,
>> + self.ifcond)
>>
>>
>> class QAPISchemaEvent(QAPISchemaEntity):
>> @@ -1462,7 +1484,8 @@ class QAPISchemaEvent(QAPISchemaEntity):
>> raise QAPISemError(self.info, "Use of 'boxed' requires 'data'")
>>
>> def visit(self, visitor):
>> - visitor.visit_event(self.name, self.info, self.arg_type, self.boxed)
>> + visitor.visit_event(self.name, self.info, self.arg_type, self.boxed,
>> + self.ifcond)
>>
>>
>> class QAPISchema(object):
>> @@ -1481,11 +1504,12 @@ class QAPISchema(object):
>> print >>sys.stderr, err
>> exit(1)
>>
>> - def _def_entity(self, ent):
>> + def _def_entity(self, ent, ifcond=None):
>> # Only the predefined types are allowed to not have info
>> assert ent.info or self._predefining
>> assert ent.name not in self._entity_dict
>> self._entity_dict[ent.name] = ent
>> + ent.set_ifcond(ifcond)
>
> .set_ifcond(None) does the right thing.
>
> However:
>
>>
>> def lookup_entity(self, name, typ=None):
>> ent = self._entity_dict.get(name)
>> @@ -1534,11 +1558,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
>
> Why is ifcond not a constructor argument like name, info, and so forth?
> What makes it special?
>
I think it was easier that way (builder pattern), but it make sense as
constructor too. Changed
>>
>> def _make_array_type(self, element_type, info):
>> @@ -1547,22 +1571,26 @@ 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):
>> + if self.lookup_entity(name, QAPISchemaObjectType):
>> + assert ifcond is None
>> + else:
>> self._def_entity(QAPISchemaObjectType(name, info, doc, None,
>> - members, None))
>> + members, None), ifcond)
>> return name
>
> Hmm, this smells like it might be the "variants objects types" mentioned
> in the commit message.
>
> Types made with _make_implicit_object_type():
>
> * The wrapper around "simple" union members, by _make_simple_variant()
>
> * A flat union's base type when it's implicit, by _def_union_type()
>
> * A command's argument type when it's implicit, by _def_command()
>
> * An event's argument type when it's implicit, by _def_event()
>
> Only the first one can be used more than once, namely when a type occurs
> in more than one simple union. The "correct" ifcond is the disjunction
> of all its user's ifconds. You make it use the *first* ifcond instead.
> Wrong: breaks when one of the other simple unions has a condition that
> is true when the first one's is false.
>
> Your commit message suggests you intended to make it unconditional
> instead. That would work: the worst that can happen is compiling a few
> q_obj_FOO_wrapper typedefs and visit_type_q_FOO_wrapper() functions that
> aren't actually used. Tolerable, in particular since I hope to get rid
> of "simple" unions some day.
>
> Sadly, it would prevent us from making the visit functions for implicit
> types static, because unused static functions trigger warnings. Let's
> not worry about that now.
>
> Generating the disjunction of all conditions wouldn't be terribly hard.
> I'm not asking for it, though.
I suggest to tackle it as follow-up then. Added a FIXME
>
> You assert that implicit types are unconditional from the second use on.
> I guess you mean to assert the ones used more than once are
> unconditional:
>
> typ = self.lookup_entity(name, QAPISchemaObjectType)
> if typ:
> assert ifcond is None and typ.ifcond is None
>
> But what you *should* assert is that the conditions are the same:
>
> typ = self.lookup_entity(name, QAPISchemaObjectType)
> if typ:
> assert ifcond == typ.ifcond
>
>>
ok
>> def _def_enum_type(self, expr, info, doc):
>> name = expr['enum']
>> data = expr['data']
>> prefix = expr.get('prefix')
>> - self._def_entity(QAPISchemaEnumType(
>> - name, info, doc, self._make_enum_members(data), prefix))
>> + return self._def_entity(QAPISchemaEnumType(
>> + name, info, doc, self._make_enum_members(data), prefix),
>> + expr.get('if'))
>
> Covers enumeration types.
>
> Why return? The (only) caller throws the value away...
left-over, fixed
>
>>
>> def _make_member(self, name, typ, info):
>> optional = False
>> @@ -1584,7 +1612,8 @@ class QAPISchema(object):
>> data = expr['data']
>> self._def_entity(QAPISchemaObjectType(name, info, doc, base,
>> self._make_members(data,
>> info),
>> - None))
>> + None),
>> + expr.get('if'))
>
> Covers struct types.
>
>>
>> def _make_variant(self, case, typ):
>> return QAPISchemaObjectTypeVariant(case, typ)
>> @@ -1593,8 +1622,10 @@ 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)
>
> A simple union member's wrapper type inherits its condition from the
> member type.
>
> I think you need to pass None instead of type_entity.ifcond here.
That doesn't work, visitor symbols are missing in generated code. Why
shouldn't the wrapper share the same condition as the underlying type?
>
>>
>> def _def_union_type(self, expr, info, doc):
>> @@ -1604,8 +1635,9 @@ class QAPISchema(object):
>> 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),
>> + expr.get('if'))
>
> A flat union's implicit base type inherits its condition from the flat
> union. Good.
>
>> if tag_name:
>> variants = [self._make_variant(key, value)
>> for (key, value) in data.iteritems()]
>> @@ -1614,14 +1646,16 @@ 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],
>> + expr.get('if'))
>
> A flat union's implicit enumeration type inherits its condition from the
> flat union. Good.
>
>> tag_member = QAPISchemaObjectTypeMember('type', typ, False)
>> members = [tag_member]
>> self._def_entity(
>> QAPISchemaObjectType(name, info, doc, base, members,
>> QAPISchemaObjectTypeVariants(tag_name,
>> tag_member,
>> - variants)))
>> + variants)),
>> + expr.get('if'))
>
> Covers union types.
>
> Third use of expr.get('if') in this function. Please put it in a
> variable.
>
> Actually, do that for *all* uses of expr[X] and expr.get(X) in class
> QAPISchema, because that's how the existing code works.
done
>
>>
>> def _def_alternate_type(self, expr, info, doc):
>> name = expr['alternate']
>> @@ -1633,7 +1667,8 @@ class QAPISchema(object):
>> QAPISchemaAlternateType(name, info, doc,
>> QAPISchemaObjectTypeVariants(None,
>> tag_member,
>> - variants)))
>> + variants)),
>> + expr.get('if'))
>
> Covers alternate types.
>
>>
>> def _def_command(self, expr, info, doc):
>> name = expr['command']
>> @@ -1644,12 +1679,14 @@ class QAPISchema(object):
>> boxed = expr.get('boxed', False)
>> 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),
>> + expr.get('if'))
>
> A command's implicit argument type inherits its condition from the
> command. Good.
>
>> 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),
>> + expr.get('if'))
>
> Covers commands.
>
>>
>> def _def_event(self, expr, info, doc):
>> name = expr['event']
>> @@ -1657,8 +1694,10 @@ class QAPISchema(object):
>> boxed = expr.get('boxed', False)
>> 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),
>> + expr.get('if'))
>
> An event's implicit argument type inherits its condition from the
> command. Good.
>
>> + self._def_entity(QAPISchemaEvent(name, info, doc, data, boxed),
>> + expr.get('if'))
>
> Covers events. You got them all. Good.
>
>>
>> def _def_exprs(self):
>> for expr_elem in self.exprs:
>> @@ -1848,6 +1887,54 @@ def guardend(name):
>> name=guardname(name))
>>
>>
>> +def gen_if(ifcond, func=''):
>> + if not ifcond:
>> + return ''
>> + if isinstance(ifcond, str):
>> + ifcond = [ifcond]
>> + ret = '\n'
>> + for ifc in ifcond:
>> + ret += mcgen('#if %(ifcond)s /* %(func)s */\n', ifcond=ifc,
>> func=func)
>> + ret += '\n'
>> + return ret
>
> Please use mcgen() like the existing code:
>
mcgen() does indent and fails with pre-processor lines. I added a comment
> ret += mcgen('''
> #if %(ifcond)s /* %(func)s */
> ''', ifcond=ifc, func=func)
>
> With the default value of @func, we get a useless, ugly comment /* */.
> If this can happen, please suppress the comment. Else, drop @func's
> default value.
>
> Lists appear to be conjunctions. What for?
I added some doc in qapi-code-gen.txt
>
>> +
>> +
>> +def gen_endif(ifcond, func=''):
>> + if not ifcond:
>> + return ''
>> + if isinstance(ifcond, str):
>> + ifcond = [ifcond]
>> + ret = '\n'
>> + for ifc in reversed(ifcond):
>> + ret += mcgen('#endif /* %(ifcond)s %(func)s */\n',
>> + ifcond=ifc, func=func)
>> + ret += '\n'
>> + return ret
>
> Likewise.
>
>> +
>> +
>> +# wrap a method to add #if / #endif to generated code
>> +# self must have 'if_members' listing the attributes to wrap
>> +# the last argument of the wrapped function must be the 'ifcond'
>
> Start your sentences with a capital letter, and end them with a period,
> please.
ok
>
>> +def if_wrap(func):
>
> Blank line, please.
ok
>
>> + def func_wrapper(self, *args, **kwargs):
>> + funcname = self.__class__.__name__ + '.' + func.__name__
>> + ifcond = args[-1]
>> + save = {}
>> + for mem in self.if_members:
>> + save[mem] = getattr(self, mem)
>> + func(self, *args, **kwargs)
>> + for mem, val in save.items():
>> + newval = getattr(self, mem)
>> + if newval != val:
>> + assert newval.startswith(val)
>> + newval = newval[len(val):]
>> + val += gen_if(ifcond, funcname)
>
> Emitting comments pointing to the QAPI schema into the generated code is
> often helpful. But this one points to QAPI generator code. Why is that
> useful?
That was mostly useful during development, removed
>
>> + val += newval
>> + val += gen_endif(ifcond, funcname)
>> + setattr(self, mem, val)
>> +
>> + return func_wrapper
>> +
>
> pep8 again:
>
> scripts/qapi.py:1955:1: E302 expected 2 blank lines, found 1
>
> Peeking ahead: this function is used as a decorator. Let's help the
> reader and mention that in the function comment, or by naming the
> function suitably. ifcond_decorator?
done
>
> Messing with the wrapped method's class's attributes is naughty. Worse,
> it's hard to understand. What alternatives have you considered?
Well, I started writing the code that checked if members got code
added, I had to put some enter()/leave() code everywhere. Then I
realize this could easily be the job for a decorator. I think the end
result is pretty neat. If you have a better idea, can you do it in a
follow-up?
>
>> def gen_enum_lookup(name, values, prefix=None):
>> ret = mcgen('''
>>
>> diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
>> index 974d0a4a80..19b1bb9b88 100644
>> --- a/scripts/qapi-commands.py
>> +++ b/scripts/qapi-commands.py
>> @@ -228,6 +228,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>> self.defn = None
>> self._regy = None
>> self._visited_ret_types = None
>> + self.if_members = ['decl', 'defn', '_regy']
>>
>> def visit_begin(self, schema):
>> self.decl = ''
>> @@ -240,8 +241,9 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor):
>> self._regy = None
>> self._visited_ret_types = None
>>
>> + @if_wrap
>> def visit_command(self, name, info, arg_type, ret_type,
>> - gen, success_response, boxed):
>> + gen, success_response, boxed, ifcond):
>> if not gen:
>> return
>> self.decl += gen_command_decl(name, arg_type, boxed, ret_type)
>> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
>> index bcbef1035f..cad9ece790 100644
>> --- a/scripts/qapi-event.py
>> +++ b/scripts/qapi-event.py
>> @@ -152,6 +152,7 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>> self.decl = None
>> self.defn = None
>> self._event_names = None
>> + self.if_members = ['decl', 'defn']
>>
>> def visit_begin(self, schema):
>> self.decl = ''
>> @@ -163,7 +164,8 @@ class QAPISchemaGenEventVisitor(QAPISchemaVisitor):
>> self.defn += gen_enum_lookup(event_enum_name, self._event_names)
>> self._event_names = None
>>
>> - def visit_event(self, name, info, arg_type, boxed):
>> + @if_wrap
>> + def visit_event(self, name, info, arg_type, boxed, ifcond):
>> self.decl += gen_event_send_decl(name, arg_type, boxed)
>> self.defn += gen_event_send(name, arg_type, boxed)
>> self._event_names.append(name)
>> diff --git a/scripts/qapi-introspect.py b/scripts/qapi-introspect.py
>> index fc72cdb66d..ecfb0f2752 100644
>> --- a/scripts/qapi-introspect.py
>> +++ b/scripts/qapi-introspect.py
>> @@ -12,7 +12,7 @@
>> from qapi import *
>>
>>
>> -def to_qlit(obj, level=0, first_indent=True):
>> +def to_qlit(obj, level=0, first_indent=True, suffix=''):
>> def indent(level):
>> return level * 4 * ' '
>> ret = ''
>> @@ -20,14 +20,20 @@ def to_qlit(obj, level=0, first_indent=True):
>> ret += indent(level)
>> if obj is None:
>> ret += 'QLIT_QNULL'
>> + elif isinstance(obj, tuple):
>> + obj, ifcond = obj
>> + ret += gen_if(ifcond)
>> + ret += to_qlit(obj, level, False) + suffix
>
> Keyword argument instead of bare False, please.
ok
>
>> + ret += gen_endif(ifcond)
>> + suffix = ''
>
> New case tuple, for generating conditionals. Okay.
>
>> elif isinstance(obj, str):
>> ret += 'QLIT_QSTR(' + '"' + obj.replace('"', r'\"') + '"' + ')'
>> elif isinstance(obj, list):
>> - elts = [to_qlit(elt, level + 1)
>> + elts = [to_qlit(elt, level + 1, True, ",")
>
> Make that
>
> elts = [to_qlit(elt, level + 1, suffix=",")
ok
>
>> for elt in obj]
>> elts.append(indent(level + 1) + "{ }")
>> ret += 'QLIT_QLIST(((QLitObject[]) {\n'
>> - ret += ',\n'.join(elts) + '\n'
>> + ret += '\n'.join(elts) + '\n'
>> ret += indent(level) + '}))'
>> elif isinstance(obj, dict):
>> elts = [ indent(level + 1) + '{ "%s", %s }' %
>> @@ -39,7 +45,7 @@ def to_qlit(obj, level=0, first_indent=True):
>> ret += indent(level) + '}))'
>> else:
>> assert False # not implemented
>> - return ret
>> + return ret + suffix
>
> This is getting really hard to review --- my brain is about to overflow
> and shut down for the day. Can you split off some preparatory work?
> The introduction of suffix here, perhaps?
ok
>
>>
>>
>> class QAPISchemaGenIntrospectVisitor(QAPISchemaVisitor):
>> @@ -113,12 +119,12 @@ const QLitObject %(c_name)s = %(c_string)s;
>> return '[' + self._use_type(typ.element_type) + ']'
>> return self._name(typ.name)
>>
>> - def _gen_qlit(self, name, mtype, obj):
>> + def _gen_qlit(self, name, mtype, obj, ifcond):
>> if mtype not in ('command', 'event', 'builtin', 'array'):
>> name = self._name(name)
>> obj['name'] = name
>> obj['meta-type'] = mtype
>> - self._qlits.append(obj)
>> + self._qlits.append((obj, ifcond))
>>
>> def _gen_member(self, member):
>> ret = {'name': member.name, 'type': self._use_type(member.type)}
>> @@ -134,38 +140,40 @@ const QLitObject %(c_name)s = %(c_string)s;
>> return {'case': variant.name, 'type': self._use_type(variant.type)}
>>
>> def visit_builtin_type(self, name, info, json_type):
>> - self._gen_qlit(name, 'builtin', {'json-type': json_type})
>> + self._gen_qlit(name, 'builtin', {'json-type': json_type}, None)
>>
>> - def visit_enum_type(self, name, info, values, prefix):
>> - self._gen_qlit(name, 'enum', {'values': values})
>> + def visit_enum_type(self, name, info, values, prefix, ifcond):
>> + self._gen_qlit(name, 'enum', {'values': values}, ifcond)
>>
>> - def visit_array_type(self, name, info, element_type):
>> + def visit_array_type(self, name, info, element_type, ifcond):
>> element = self._use_type(element_type)
>> - self._gen_qlit('[' + element + ']', 'array', {'element-type':
>> element})
>> + self._gen_qlit('[' + element + ']', 'array', {'element-type':
>> element},
>> + ifcond)
>>
>> - def visit_object_type_flat(self, name, info, members, variants):
>> + def visit_object_type_flat(self, name, info, members, variants, ifcond):
>> obj = {'members': [self._gen_member(m) for m in members]}
>> if variants:
>> obj.update(self._gen_variants(variants.tag_member.name,
>> variants.variants))
>> - self._gen_qlit(name, 'object', obj)
>> + self._gen_qlit(name, 'object', obj, ifcond)
>>
>> - def visit_alternate_type(self, name, info, variants):
>> + def visit_alternate_type(self, name, info, variants, ifcond):
>> self._gen_qlit(name, 'alternate',
>> {'members': [{'type': self._use_type(m.type)}
>> - for m in variants.variants]})
>> + for m in variants.variants]}, ifcond)
>>
>> def visit_command(self, name, info, arg_type, ret_type,
>> - gen, success_response, boxed):
>> + gen, success_response, boxed, ifcond):
>> arg_type = arg_type or self._schema.the_empty_object_type
>> ret_type = ret_type or self._schema.the_empty_object_type
>> self._gen_qlit(name, 'command',
>> {'arg-type': self._use_type(arg_type),
>> - 'ret-type': self._use_type(ret_type)})
>> + 'ret-type': self._use_type(ret_type)}, ifcond)
>>
>> - def visit_event(self, name, info, arg_type, boxed):
>> + def visit_event(self, name, info, arg_type, boxed, ifcond):
>> arg_type = arg_type or self._schema.the_empty_object_type
>> - self._gen_qlit(name, 'event', {'arg-type':
>> self._use_type(arg_type)})
>> + self._gen_qlit(name, 'event', {'arg-type':
>> self._use_type(arg_type)},
>> + ifcond)
>>
>> # Debugging aid: unmask QAPI schema's type names
>> # We normally mask them, because they're not QMP wire ABI
>
> Out of review brainpower for today. Hope to resume tomorrow.
>
> [...]
>
--
Marc-André Lureau