| Message ID | 20170727154126.11339-8-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
Marc-André Lureau <marcandre.lureau@redhat.com> 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. > Variants objects types are created outside of #if blocks, since they What are "variants objects types"? > 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 <marcandre.lureau@redhat.com> > --- > 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. Also, pep8 gripes: scripts/qapi.py:647:9: E129 visually indented line with same indent as next logical line > + > + > 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. > 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. > > 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. > > > 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? > > 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. 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 > > 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... > > 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. > > 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. > > 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: 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? > + > + > +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. > +def if_wrap(func): Blank line, please. > + 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? > + 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? Messing with the wrapped method's class's attributes is naughty. Worse, it's hard to understand. What alternatives have you considered? > 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. > + 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=",") > 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? > > > 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. [...]
Markus Armbruster <armbru@redhat.com> writes: [...] > Out of review brainpower for today. Hope to resume tomorrow. > > [...] Nope, I'm giving up on this one. Please split it for reviewability. Suggested split: 1. Preparatory refactoring for step 2, step by step 2. Frontend part: accept and check 'if', step by step The accepted conditions should be visible in tests/qapi-schema/qapi-schema-test.out. 3. Preparatory refactoring for step 4, step by step 4. Backend part: generate the ifdeffery, step by step You already split off qapi2texi steps [PATCH 12-15]. Good. Perhaps that's all that can be split off, perhaps not. PATCH 08-11 may well profit from the same treatment. Moving on to PATCH 16.
Marc-André Lureau <marcandre.lureau@redhat.com> writes: > Add 'if' c-preprocessor condition on top-level schema elements: > struct, enum, union, alternate, command, event. > > Variants objects types are created outside of #if blocks, since they > 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 > 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. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > 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 Missing: docs/devel/qapi-code-gen.txt update. [...]
Hi On Wed, Aug 16, 2017 at 5:43 PM, Markus Armbruster <armbru@redhat.com> wrote: > Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com> >> --- >> 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 <marcandre.lureau@gmail.com> writes: > Hi > > On Wed, Aug 16, 2017 at 5:43 PM, Markus Armbruster <armbru@redhat.com> wrote: >> Marc-André Lureau <marcandre.lureau@redhat.com> 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 <marcandre.lureau@redhat.com> >>> --- >>> 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? Because it's shared with other simple unions that have the same variant? >>> >>> 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 I see. Comment is good enough for now. >> 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. I think "clever" would describe it better than "neat". Possibly "too clever". > If you have a better idea, can you do it in a > follow-up? I need to complete review before I can tell. >> >>> def gen_enum_lookup(name, values, prefix=None): >>> ret = mcgen(''' >>> [...]
On Tue, Aug 22, 2017 at 06:52:19PM +0200, Markus Armbruster wrote: > Marc-André Lureau <marcandre.lureau@gmail.com> writes: [...] > >>> + 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. > > I think "clever" would describe it better than "neat". Possibly "too > clever". FWIW, I was investigating something else in the series and took a while to understand how the #if lines were magically appearing on self.decl and self.defn. I'd prefer something simpler like: def cond(ifcond, s): if s: return gen_if(ifcond) + s + gen_endif(ifcond) return s def visit_command(self, name, info, arg_type, ret_type, gen, success_response, boxed, ifcond): if not gen: return self.decl += cond(ifcond, gen_command_decl(name, arg_type, boxed, ret_type)) if ret_type and ret_type not in self._visited_ret_types: self._visited_ret_types.add(ret_type) self.defn += cond(ifcond,gen_marshal_output(ret_type)) self.decl += cond(ifcond, gen_marshal_decl(name)) self.defn += cond(ifcond, gen_marshal(name, arg_type, boxed, ret_type)) self._regy += cond(ifcond, gen_register_command(name, success_response)) If all callers of some of the gen_*() functions above are wrapped by ifcond(), then a decorator around them could still be useful. But preferably if the decorator affects only the function arguments and return value, not messing with the object attributes or other state outside the function.
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") + + 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') 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 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) 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) 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 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 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')) 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')) 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) 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')) 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')) 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')) 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')) 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')) 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')) 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')) + self._def_entity(QAPISchemaEvent(name, info, doc, data, boxed), + expr.get('if')) 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 + + +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 + + +# 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' +def if_wrap(func): + 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) + val += newval + val += gen_endif(ifcond, funcname) + setattr(self, mem, val) + + return func_wrapper + 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 + ret += gen_endif(ifcond) + suffix = '' 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, ",") 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 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 diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py index b45e7b5634..d0d2eb917c 100644 --- a/scripts/qapi-types.py +++ b/scripts/qapi-types.py @@ -53,19 +53,22 @@ def gen_struct_members(members): return ret -def gen_object(name, base, members, variants): - if name in objects_seen: - return '' - objects_seen.add(name) - +def gen_variants_objects(variants): ret = '' if variants: for v in variants.variants: if isinstance(v.type, QAPISchemaObjectType): + ret += gen_variants_objects(v.type.variants) ret += gen_object(v.type.name, v.type.base, v.type.local_members, v.type.variants) + return ret - ret += mcgen(''' +def gen_object(name, base, members, variants): + if name in objects_seen: + return '' + objects_seen.add(name) + + ret = mcgen(''' struct %(c_name)s { ''', @@ -171,6 +174,7 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self.defn = None self._fwdecl = None self._btin = None + self.if_members = ['decl', 'defn', '_fwdecl', '_btin'] def visit_begin(self, schema): # gen_object() is recursive, ensure it doesn't visit the empty type @@ -191,11 +195,13 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self.decl = self._btin + self.decl self._btin = None - def _gen_type_cleanup(self, name): + @if_wrap + def _gen_type_cleanup(self, name, ifcond): self.decl += gen_type_cleanup_decl(name) self.defn += gen_type_cleanup(name) - def visit_enum_type(self, name, info, values, prefix): + @if_wrap + def visit_enum_type(self, name, info, values, prefix, ifcond): # Special case for our lone builtin enum type # TODO use something cleaner than existence of info if not info: @@ -206,7 +212,8 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): self._fwdecl += gen_enum(name, values, prefix) self.defn += gen_enum_lookup(name, values, prefix) - def visit_array_type(self, name, info, element_type): + @if_wrap + def visit_array_type(self, name, info, element_type, ifcond): if isinstance(element_type, QAPISchemaBuiltinType): self._btin += gen_fwd_object_or_array(name) self._btin += gen_array(name, element_type) @@ -216,13 +223,10 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): else: self._fwdecl += gen_fwd_object_or_array(name) self.decl += gen_array(name, element_type) - self._gen_type_cleanup(name) + self._gen_type_cleanup(name, ifcond) - def visit_object_type(self, name, info, base, members, variants): - # Nothing to do for the special empty builtin - if name == 'q_empty': - return - self._fwdecl += gen_fwd_object_or_array(name) + @if_wrap + def _gen_object(self, name, info, base, members, variants, ifcond): self.decl += gen_object(name, base, members, variants) if base and not base.is_implicit(): self.decl += gen_upcast(name, base) @@ -230,12 +234,21 @@ class QAPISchemaGenTypeVisitor(QAPISchemaVisitor): # directly use rather than repeat type.is_implicit()? if not name.startswith('q_'): # implicit types won't be directly allocated/freed - self._gen_type_cleanup(name) + self._gen_type_cleanup(name, ifcond) + + def visit_object_type(self, name, info, base, members, variants, ifcond): + # Nothing to do for the special empty builtin + if name == 'q_empty': + return + self._fwdecl += gen_fwd_object_or_array(name) + self.decl += gen_variants_objects(variants) + self._gen_object(name, info, base, members, variants, ifcond) - def visit_alternate_type(self, name, info, variants): + def visit_alternate_type(self, name, info, variants, ifcond): self._fwdecl += gen_fwd_object_or_array(name) - self.decl += gen_object(name, None, [variants.tag_member], variants) - self._gen_type_cleanup(name) + self.decl += gen_variants_objects(variants) + self._gen_object(name, info, None, [variants.tag_member], + variants, ifcond) # If you link code generated from multiple schemata, you want only one # instance of the code for built-in types. Generate it only when diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py index 60850a6cdd..335407d078 100644 --- a/scripts/qapi-visit.py +++ b/scripts/qapi-visit.py @@ -267,6 +267,7 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): self.decl = None self.defn = None self._btin = None + self.if_members = ['decl', 'defn', '_btin'] def visit_begin(self, schema): self.decl = '' @@ -282,7 +283,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): self.decl = self._btin + self.decl self._btin = None - def visit_enum_type(self, name, info, values, prefix): + @if_wrap + def visit_enum_type(self, name, info, values, prefix, ifcond): # Special case for our lone builtin enum type # TODO use something cleaner than existence of info if not info: @@ -293,7 +295,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): self.decl += gen_visit_decl(name, scalar=True) self.defn += gen_visit_enum(name, prefix) - def visit_array_type(self, name, info, element_type): + @if_wrap + def visit_array_type(self, name, info, element_type, ifcond): decl = gen_visit_decl(name) defn = gen_visit_list(name, element_type) if isinstance(element_type, QAPISchemaBuiltinType): @@ -304,7 +307,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): self.decl += decl self.defn += defn - def visit_object_type(self, name, info, base, members, variants): + @if_wrap + def visit_object_type(self, name, info, base, members, variants, ifcond): # Nothing to do for the special empty builtin if name == 'q_empty': return @@ -317,7 +321,8 @@ class QAPISchemaGenVisitVisitor(QAPISchemaVisitor): self.decl += gen_visit_decl(name) self.defn += gen_visit_object(name, base, members, variants) - def visit_alternate_type(self, name, info, variants): + @if_wrap + def visit_alternate_type(self, name, info, variants, ifcond): self.decl += gen_visit_decl(name) self.defn += gen_visit_alternate(name, variants) diff --git a/scripts/qapi2texi.py b/scripts/qapi2texi.py index 639eb1d042..5aa2f48bfd 100755 --- a/scripts/qapi2texi.py +++ b/scripts/qapi2texi.py @@ -207,7 +207,7 @@ class QAPISchemaGenDocVisitor(qapi.QAPISchemaVisitor): def visit_begin(self, schema): self.out = '' - def visit_enum_type(self, name, info, values, prefix): + def visit_enum_type(self, name, info, values, prefix, ifcond): doc = self.cur_doc if self.out: self.out += '\n' @@ -216,7 +216,7 @@ class QAPISchemaGenDocVisitor(qapi.QAPISchemaVisitor): body=texi_entity(doc, 'Values', member_func=texi_enum_value)) - def visit_object_type(self, name, info, base, members, variants): + def visit_object_type(self, name, info, base, members, variants, ifcond): doc = self.cur_doc if base and base.is_implicit(): base = None @@ -226,7 +226,7 @@ class QAPISchemaGenDocVisitor(qapi.QAPISchemaVisitor): name=doc.symbol, body=texi_entity(doc, 'Members', base, variants)) - def visit_alternate_type(self, name, info, variants): + def visit_alternate_type(self, name, info, variants, ifcond): doc = self.cur_doc if self.out: self.out += '\n' @@ -235,7 +235,7 @@ class QAPISchemaGenDocVisitor(qapi.QAPISchemaVisitor): body=texi_entity(doc, 'Members')) def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, ifcond): doc = self.cur_doc if self.out: self.out += '\n' @@ -249,7 +249,7 @@ class QAPISchemaGenDocVisitor(qapi.QAPISchemaVisitor): name=doc.symbol, body=body) - def visit_event(self, name, info, arg_type, boxed): + def visit_event(self, name, info, arg_type, boxed, ifcond): doc = self.cur_doc if self.out: self.out += '\n' diff --git a/tests/Makefile.include b/tests/Makefile.include index 960ab8c6dd..0fc7088b2c 100644 --- a/tests/Makefile.include +++ b/tests/Makefile.include @@ -373,6 +373,7 @@ qapi-schema += args-unknown.json qapi-schema += bad-base.json qapi-schema += bad-data.json qapi-schema += bad-ident.json +qapi-schema += bad-if.json qapi-schema += bad-type-bool.json qapi-schema += bad-type-dict.json qapi-schema += bad-type-int.json diff --git a/tests/qapi-schema/bad-if.err b/tests/qapi-schema/bad-if.err new file mode 100644 index 0000000000..8054fbb143 --- /dev/null +++ b/tests/qapi-schema/bad-if.err @@ -0,0 +1 @@ +tests/qapi-schema/bad-if.json:2: 'if' condition requires a string or a list of string diff --git a/tests/qapi-schema/bad-if.exit b/tests/qapi-schema/bad-if.exit new file mode 100644 index 0000000000..d00491fd7e --- /dev/null +++ b/tests/qapi-schema/bad-if.exit @@ -0,0 +1 @@ +1 diff --git a/tests/qapi-schema/bad-if.json b/tests/qapi-schema/bad-if.json new file mode 100644 index 0000000000..3edd1a0bf2 --- /dev/null +++ b/tests/qapi-schema/bad-if.json @@ -0,0 +1,3 @@ +# check invalid 'if' type +{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' }, + 'if': { 'value': 'defined(TEST_IF_STRUCT)' } } diff --git a/tests/qapi-schema/bad-if.out b/tests/qapi-schema/bad-if.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json index c72dbd8050..dc2c444fc1 100644 --- a/tests/qapi-schema/qapi-schema-test.json +++ b/tests/qapi-schema/qapi-schema-test.json @@ -188,3 +188,23 @@ 'data': { 'a': ['__org.qemu_x-Enum'], 'b': ['__org.qemu_x-Struct'], 'c': '__org.qemu_x-Union2', 'd': '__org.qemu_x-Alt' }, 'returns': '__org.qemu_x-Union1' } + +# test 'if' condition handling + +{ 'struct': 'TestIfStruct', 'data': { 'foo': 'int' }, + 'if': 'defined(TEST_IF_STRUCT)' } + +{ 'enum': 'TestIfEnum', 'data': [ 'foo', 'bar' ], + 'if': 'defined(TEST_IF_ENUM)' } + +{ 'union': 'TestIfUnion', 'data': { 'foo': 'TestStruct' }, + 'if': 'defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT)' } + +{ 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar': 'TestStruct' }, + 'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' } + +{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' }, + 'if': 'defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)' } + +{ 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' }, + 'if': 'defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)' } diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out index 3b1e9082d3..fc5fd25f1b 100644 --- a/tests/qapi-schema/qapi-schema-test.out +++ b/tests/qapi-schema/qapi-schema-test.out @@ -52,6 +52,29 @@ enum QEnumTwo ['value1', 'value2'] prefix QENUM_TWO enum QType ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist', 'qbool'] prefix QTYPE +alternate TestIfAlternate + tag type + case foo: int + case bar: TestStruct + if defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT) +command TestIfCmd q_obj_TestIfCmd-arg -> None + gen=True success_response=True boxed=False + if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT) +enum TestIfEnum ['foo', 'bar'] + if defined(TEST_IF_ENUM) +event TestIfEvent q_obj_TestIfEvent-arg + boxed=False + if defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT) +object TestIfStruct + member foo: int optional=False + if defined(TEST_IF_STRUCT) +object TestIfUnion + member type: TestIfUnionKind optional=False + tag type + case foo: q_obj_TestStruct-wrapper + if defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT) +enum TestIfUnionKind ['foo'] + if defined(TEST_IF_UNION) && defined(TEST_IF_STRUCT) object TestStruct member integer: int optional=False member boolean: bool optional=False @@ -172,6 +195,14 @@ object q_obj_EVENT_D-arg member b: str optional=False member c: str optional=True member enum3: EnumOne optional=True +object q_obj_TestIfCmd-arg + member foo: TestIfStruct optional=False + if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT) +object q_obj_TestIfEvent-arg + member foo: TestIfStruct optional=False + if defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT) +object q_obj_TestStruct-wrapper + member data: TestStruct optional=False object q_obj_UserDefFlatUnion2-base member integer: int optional=True member string: str optional=False diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py index c7724d3437..17fd975812 100644 --- a/tests/qapi-schema/test-qapi.py +++ b/tests/qapi-schema/test-qapi.py @@ -17,12 +17,13 @@ import sys class QAPISchemaTestVisitor(QAPISchemaVisitor): - def visit_enum_type(self, name, info, values, prefix): + def visit_enum_type(self, name, info, values, prefix, ifcond): print 'enum %s %s' % (name, values) if prefix: print ' prefix %s' % prefix + self._print_if(ifcond) - def visit_object_type(self, name, info, base, members, variants): + def visit_object_type(self, name, info, base, members, variants, ifcond): print 'object %s' % name if base: print ' base %s' % base.name @@ -30,21 +31,25 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): print ' member %s: %s optional=%s' % \ (m.name, m.type.name, m.optional) self._print_variants(variants) + self._print_if(ifcond) - def visit_alternate_type(self, name, info, variants): + def visit_alternate_type(self, name, info, variants, ifcond): print 'alternate %s' % name self._print_variants(variants) + self._print_if(ifcond) def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, ifcond): print 'command %s %s -> %s' % \ (name, arg_type and arg_type.name, ret_type and ret_type.name) print ' gen=%s success_response=%s boxed=%s' % \ (gen, success_response, boxed) + self._print_if(ifcond) - def visit_event(self, name, info, arg_type, boxed): + def visit_event(self, name, info, arg_type, boxed, ifcond): print 'event %s %s' % (name, arg_type and arg_type.name) print ' boxed=%s' % boxed + self._print_if(ifcond) @staticmethod def _print_variants(variants): @@ -53,6 +58,12 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): for v in variants.variants: print ' case %s: %s' % (v.name, v.type.name) + @staticmethod + def _print_if(ifcond): + if ifcond: + print ' if %s' % ifcond + + schema = QAPISchema(sys.argv[1]) schema.visit(QAPISchemaTestVisitor())
Add 'if' c-preprocessor condition on top-level schema elements: struct, enum, union, alternate, command, event. Variants objects types are created outside of #if blocks, since they 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 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. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- 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