[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 19/50] qapi: add 'if' to enum members
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v3 19/50] qapi: add 'if' to enum members |
Date: |
Thu, 11 Jan 2018 22:24:22 +0100 |
Hi
On Fri, Dec 8, 2017 at 9:38 AM, Markus Armbruster <address@hidden> wrote:
> A bit more detail in the commit message would make this patch easier to
> review.
>
> Marc-André Lureau <address@hidden> writes:
>
>> Signed-off-by: Marc-André Lureau <address@hidden>
>> ---
>> scripts/qapi.py | 39
>> ++++++++++++++++++++++++++++-----
>> tests/Makefile.include | 1 -
>> tests/qapi-schema/enum-dict-member.err | 1 -
>> tests/qapi-schema/enum-dict-member.exit | 1 -
>> tests/qapi-schema/enum-dict-member.json | 2 --
>> tests/qapi-schema/enum-dict-member.out | 0
>> tests/qapi-schema/qapi-schema-test.json | 5 +++--
>> tests/qapi-schema/qapi-schema-test.out | 3 ++-
>> tests/qapi-schema/test-qapi.py | 2 ++
>> 9 files changed, 41 insertions(+), 13 deletions(-)
>> delete mode 100644 tests/qapi-schema/enum-dict-member.err
>> delete mode 100644 tests/qapi-schema/enum-dict-member.exit
>> delete mode 100644 tests/qapi-schema/enum-dict-member.json
>> delete mode 100644 tests/qapi-schema/enum-dict-member.out
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 386a577a59..1535de9ce7 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -659,6 +659,14 @@ def check_if(expr, info):
>> info, "'if' condition must be a string or a list of strings")
>>
>>
>> +def check_unknown_keys(info, dict, allowed_keys):
>> + diff = set(dict) - allowed_keys
>> + if not diff:
>> + return
>> + raise QAPISemError(info, "Dictionnary has unknown keys: %s (allowed:
>> %s)" %
>
> s/Dictionnary/Dictionary/
ok
>
>> + (', '.join(diff), ', '.join(allowed_keys)))
>
> I'm afraid this duplicates a part of check_keys(). Could it be factored
> out?
done
>
>> +
>> +
>> def check_type(info, source, value, allow_array=False,
>> allow_dict=False, allow_optional=False,
>> allow_metas=[]):
>> @@ -739,6 +747,10 @@ def check_event(expr, info):
>> allow_metas=meta)
>>
>>
>> +def enum_get_values(expr):
>> + return [e if isinstance(e, str) else e['name'] for e in expr['data']]
>> +
>> +
>> def check_union(expr, info):
>> name = expr['union']
>> base = expr.get('base')
>> @@ -798,7 +810,7 @@ def check_union(expr, info):
>> # If the discriminator names an enum type, then all members
>> # of 'data' must also be members of the enum type.
>> if enum_define:
>> - if key not in enum_define['data']:
>> + if key not in enum_get_values(enum_define):
>> raise QAPISemError(info,
>> "Discriminator value '%s' is not found
>> in "
>> "enum '%s'"
>> @@ -806,7 +818,7 @@ def check_union(expr, info):
>>
>> # If discriminator is user-defined, ensure all values are covered
>> if enum_define:
>> - for value in enum_define['data']:
>> + for value in enum_get_values(enum_define):
>> if value not in members.keys():
>> raise QAPISemError(info, "Union '%s' data missing '%s'
>> branch"
>> % (name, value))
>> @@ -837,7 +849,7 @@ def check_alternate(expr, info):
>> if qtype == 'QTYPE_QSTRING':
>> enum_expr = enum_types.get(value)
>> if enum_expr:
>> - for v in enum_expr['data']:
>> + for v in enum_get_values(enum_expr):
>> if v in ['on', 'off']:
>> conflicting.add('QTYPE_QBOOL')
>> if re.match(r'[-+0-9.]', v): # lazy, could be tightened
>> @@ -865,6 +877,14 @@ def check_enum(expr, info):
>> raise QAPISemError(info,
>> "Enum '%s' requires a string for 'prefix'" %
>> name)
>> for member in members:
>> + if isinstance(member, dict):
>> + if 'name' not in member:
>> + raise QAPISemError(info, "Dictionary member of enum '%s'
>> must "
>> + "have a 'name' key" % name)
>> + if 'if' in member:
>> + check_if(member, info)
>> + check_unknown_keys(info, member, {'name', 'if'})
>> + member = member['name']
>> check_name(info, "Member of enum '%s'" % name, member,
>> enum_member=True)
>>
>> @@ -1280,9 +1300,11 @@ class QAPISchemaObjectType(QAPISchemaType):
>> class QAPISchemaMember(object):
>> role = 'member'
>>
>> - def __init__(self, name):
>> + def __init__(self, name, ifcond=None):
>> assert isinstance(name, str)
>> + assert ifcond is None or isinstance(ifcond, str)
>> self.name = name
>> + self.ifcond = ifcond
>> self.owner = None
>>
>> def set_owner(self, name):
>
> QAPISchemaObjectTypeMember inherits .ifcond. Peeking ahead: looks like
> it'll get used when we add conditions to object type members, PATCH
> 23ff. Okay. Mentioning it in the commit message wouldn't hurt, though.
It will *also* get used. Ok, updated commit message.
>
>> @@ -1559,7 +1581,14 @@ class QAPISchema(object):
>> qtype_values, 'QTYPE'))
>>
>> def _make_enum_members(self, values):
>> - return [QAPISchemaMember(v) for v in values]
>> + enum = []
>> + for v in values:
>> + ifcond = None
>> + if isinstance(v, dict):
>> + ifcond = v.get('if')
>> + v = v['name']
>> + enum.append(QAPISchemaMember(v, ifcond))
>
>
> I like brevity a lot, but if it's bought by assigning to a loop control
> variable, I pass. Cleaner:
>
> for v in values:
> if isinstance(v, dict):
> name = v['name']
> ifcond = v.get('if')
> else:
> name = v
> ifcond = None
> enum.append(QAPISchemaMember(name, ifcond))
>
sure
>> + return enum
>>
>> def _make_implicit_enum_type(self, name, info, ifcond, values):
>> # See also QAPISchemaObjectTypeMember._pretty_owner()
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index 8dac7c9083..a9f0ddbe01 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -443,7 +443,6 @@ qapi-schema += empty.json
>> qapi-schema += enum-bad-name.json
>> qapi-schema += enum-bad-prefix.json
>> qapi-schema += enum-clash-member.json
>> -qapi-schema += enum-dict-member.json
>> qapi-schema += enum-int-member.json
>> qapi-schema += enum-member-case.json
>> qapi-schema += enum-missing-data.json
>> diff --git a/tests/qapi-schema/enum-dict-member.err
>> b/tests/qapi-schema/enum-dict-member.err
>> deleted file mode 100644
>> index 8ca146ea59..0000000000
>> --- a/tests/qapi-schema/enum-dict-member.err
>> +++ /dev/null
>> @@ -1 +0,0 @@
>> -tests/qapi-schema/enum-dict-member.json:2: Member of enum 'MyEnum' requires
>> a string name
>> diff --git a/tests/qapi-schema/enum-dict-member.exit
>> b/tests/qapi-schema/enum-dict-member.exit
>> deleted file mode 100644
>> index d00491fd7e..0000000000
>> --- a/tests/qapi-schema/enum-dict-member.exit
>> +++ /dev/null
>> @@ -1 +0,0 @@
>> -1
>> diff --git a/tests/qapi-schema/enum-dict-member.json
>> b/tests/qapi-schema/enum-dict-member.json
>> deleted file mode 100644
>> index 79672e0f09..0000000000
>> --- a/tests/qapi-schema/enum-dict-member.json
>> +++ /dev/null
>> @@ -1,2 +0,0 @@
>> -# we reject any enum member that is not a string
>> -{ 'enum': 'MyEnum', 'data': [ { 'value': 'str' } ] }
>> diff --git a/tests/qapi-schema/enum-dict-member.out
>> b/tests/qapi-schema/enum-dict-member.out
>> deleted file mode 100644
>> index e69de29bb2..0000000000
>
> Hmm. The dict instance of "enum value must be of an appropriate JSON
> type" error class goes away in this patch, but the class remains. Do we
> still cover it?
>
> There's enum-int-member.json, but it's not a good test: it dies in the
> lexer. I think we should replace the two tests by a single one.
> Perhaps something like 'data': [ [] ] would do.
I replaced enum-dict-member by enum-bad-member. I left the int-member
test, because it has also the purpose to check the lexer.
>
>> diff --git a/tests/qapi-schema/qapi-schema-test.json
>> b/tests/qapi-schema/qapi-schema-test.json
>> index dc2c444fc1..ad2b405d83 100644
>> --- a/tests/qapi-schema/qapi-schema-test.json
>> +++ b/tests/qapi-schema/qapi-schema-test.json
>> @@ -194,7 +194,8 @@
>> { 'struct': 'TestIfStruct', 'data': { 'foo': 'int' },
>> 'if': 'defined(TEST_IF_STRUCT)' }
>>
>> -{ 'enum': 'TestIfEnum', 'data': [ 'foo', 'bar' ],
>> +{ 'enum': 'TestIfEnum', 'data':
>> + [ 'foo', { 'name' : 'bar', 'if': 'defined(TEST_IF_ENUM_BAR)' } ],
>> 'if': 'defined(TEST_IF_ENUM)' }
>>
>> { 'union': 'TestIfUnion', 'data': { 'foo': 'TestStruct' },
>> @@ -203,7 +204,7 @@
>> { 'alternate': 'TestIfAlternate', 'data': { 'foo': 'int', 'bar':
>> 'TestStruct' },
>> 'if': 'defined(TEST_IF_ALT) && defined(TEST_IF_STRUCT)' }
>>
>> -{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct' },
>> +{ 'command': 'TestIfCmd', 'data': { 'foo': 'TestIfStruct', 'bar':
>> 'TestIfEnum' },
>> 'if': 'defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)' }
>>
>> { 'event': 'TestIfEvent', 'data': { 'foo': 'TestIfStruct' },
>
> Should this hunk be in PATCH 04?
It could, but it is not necessary until conditionals are added to
verify introspection generation.
>
>> diff --git a/tests/qapi-schema/qapi-schema-test.out
>> b/tests/qapi-schema/qapi-schema-test.out
>> index 9a7cafc269..8a0cf1a551 100644
>> --- a/tests/qapi-schema/qapi-schema-test.out
>> +++ b/tests/qapi-schema/qapi-schema-test.out
>> @@ -74,7 +74,7 @@ command TestIfCmd q_obj_TestIfCmd-arg -> None
>> if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)
>> enum TestIfEnum
>> member foo:
>> - member bar:
>> + member bar: if=defined(TEST_IF_ENUM_BAR)
>> if defined(TEST_IF_ENUM)
>> event TestIfEvent q_obj_TestIfEvent-arg
>> boxed=False
>> @@ -228,6 +228,7 @@ object q_obj_EVENT_D-arg
>> member enum3: EnumOne optional=True
>> object q_obj_TestIfCmd-arg
>> member foo: TestIfStruct optional=False
>> + member bar: TestIfEnum optional=False
>> if defined(TEST_IF_CMD) && defined(TEST_IF_STRUCT)
>> object q_obj_TestIfEvent-arg
>> member foo: TestIfStruct optional=False
>> diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
>> index 67c6c1ecef..a86c3b5ee1 100644
>> --- a/tests/qapi-schema/test-qapi.py
>> +++ b/tests/qapi-schema/test-qapi.py
>> @@ -56,6 +56,8 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor):
>> print ' member %s:' % m.name,
>> if isinstance(m, QAPISchemaObjectTypeMember):
>> print '%s optional=%s' % (m.type.name, m.optional),
>> + if m.ifcond:
>> + print 'if=%s' % m.ifcond,
>> print
>>
>> @staticmethod
>
--
Marc-André Lureau
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v3 19/50] qapi: add 'if' to enum members,
Marc-André Lureau <=