Eric Blake <ebl...@redhat.com> writes:
> On 07/01/2015 02:22 PM, Markus Armbruster wrote:
>> The old code prints the result of parsing (list of expression
>> dictionaries), and partial results of semantic analysis (list of enum
>> dictionaries, list of struct dictionaries).
>>
>> The new code prints a trace of a schema visit, i.e. what the back-ends
>> are going to use. Built-in and array types are omitted, because
>> they're boring.
>>
>> Signed-off-by: Markus Armbruster <arm...@redhat.com>
>> ---
>> tests/qapi-schema/alternate-good.out | 15 +-
>> tests/qapi-schema/comments.out | 4 +-
>> tests/qapi-schema/data-member-array.out | 13 +-
>> tests/qapi-schema/empty.out | 3 -
>> tests/qapi-schema/enum-empty.out | 4 +-
>> tests/qapi-schema/event-case.out | 4 +-
>> tests/qapi-schema/flat-union-reverse-define.out | 21 +--
>> tests/qapi-schema/ident-with-escape.out | 7 +-
>> tests/qapi-schema/include-relpath.out | 4 +-
>> tests/qapi-schema/include-repetition.out | 4 +-
>> tests/qapi-schema/include-simple.out | 4 +-
>> tests/qapi-schema/indented-expr.out | 7 +-
>> tests/qapi-schema/qapi-schema-test.out | 186
>> +++++++++++++++++-------
>> tests/qapi-schema/returns-int.out | 5 +-
>> tests/qapi-schema/test-qapi.py | 37 ++++-
>> tests/qapi-schema/type-bypass.out | 7 +-
>> 16 files changed, 210 insertions(+), 115 deletions(-)
>
> We have a lot more negative than positive tests of the parser (good
> thing, because that meant fewer .out files to update to the new format).
>
> No change to actual qemu code, and proves that the previous three
> patches have set up enough of a framework to accurately cover our testsuite.
>
>>
>> diff --git a/tests/qapi-schema/alternate-good.out
>> b/tests/qapi-schema/alternate-good.out
>> index 99848ee..0cbdfa1 100644
>> --- a/tests/qapi-schema/alternate-good.out
>> +++ b/tests/qapi-schema/alternate-good.out
>> @@ -1,6 +1,9 @@
>> -[OrderedDict([('struct', 'Data'), ('data', OrderedDict([('*number',
>> 'int'), ('*name', 'str')]))]),
>> - OrderedDict([('enum', 'Enum'), ('data', ['hello', 'world'])]),
>> - OrderedDict([('alternate', 'Alt'), ('data', OrderedDict([('value',
>> 'int'), ('string', 'Enum'), ('struct', 'Data')]))])]
>> -[{'enum_name': 'Enum', 'enum_values': ['hello', 'world']},
>> - {'enum_name': 'AltKind', 'enum_values': None}]
>> -[OrderedDict([('struct', 'Data'), ('data', OrderedDict([('*number',
>> 'int'), ('*name', 'str')]))])]
>> +alternate Alt
>> + case value: int flat=False
>> + case string: Enum flat=False
>> + case struct: Data flat=False
>
> I'm still not convinced whether we need .flat exposed through this much
> detail, or if we should just normalize plain unions into flat unions
> with implicit structs for each branch. Changing your design will have
> obvious ripple effects here.
Yes. I think it's okay as long as we keep it out of external
interfaces.
>> +++ b/tests/qapi-schema/data-member-array.out
>> @@ -1,5 +1,8 @@
>> -[OrderedDict([('enum', 'abc'), ('data', ['a', 'b', 'c'])]),
>> - OrderedDict([('struct', 'def'), ('data', OrderedDict([('array',
>> ['abc'])]))]),
>> - OrderedDict([('command', 'okay'), ('data',
>> OrderedDict([('member1', ['int']), ('member2', ['def'])]))])]
>> -[{'enum_name': 'abc', 'enum_values': ['a', 'b', 'c']}]
>> -[OrderedDict([('struct', 'def'), ('data', OrderedDict([('array',
>> ['abc'])]))])]
>> +object :obj-okay-args
>> + member member1: intList optional=False
>
> Took me a moment to realize the object is an implicit one (named
> ':obj-okay-args') and not a typo for 'object: obj-okay-args' consistent
> with members being listed 'name: type'. But not worth changing things,
> as it is sufficiently unambiguous to serve as a valid test.
Perhaps omitting the ':' after member names would be less confusing.
>> +object UserDefFlatUnion
>> + base UserDefUnionBase
>> + tag enum1
>> + case value1: UserDefA flat=True
>> + case value2: UserDefB flat=True
>> + case value3: UserDefB flat=True
>> +object UserDefFlatUnion2
>> + base UserDefUnionBase
>> + tag enum1
>> + case value1: UserDefC flat=True
>> + case value2: UserDefB flat=True
>> + case value3: UserDefA flat=True
>> +object UserDefNativeListUnion
>> + case integer: intList flat=False
>> + case s8: int8List flat=False
>> + case s16: int16List flat=False
>> + case s32: int32List flat=False
>> + case s64: int64List flat=False
>> + case u8: uint8List flat=False
>> + case u16: uint16List flat=False
>> + case u32: uint32List flat=False
>> + case u64: uint64List flat=False
>> + case number: numberList flat=False
>> + case boolean: boolList flat=False
>> + case string: strList flat=False
>> + case sizes: sizeList flat=False
>> +enum UserDefNativeListUnionKind ['integer', 's8', 's16', 's32',
>> 's64', 'u8', 'u16', 'u32', 'u64', 'number', 'boolean', 'string',
>> 'sizes']
>
> Hmm. You are dumping the tag name and type of flat unions, but not of
> simple unions. I would have expected:
>
> object UserDefNativeListUnion
> member kind: UserDefNativeListUnionKind
> tag kind
> case integer: intList flat=False
> ...
See below.
> The above was fallout, while below is the meat of the new visitor.
>
>> +++ b/tests/qapi-schema/test-qapi.py
>> @@ -15,11 +15,34 @@ from pprint import pprint
>> import os
>> import sys
>>
>> -try:
>> - exprs = QAPISchema(sys.argv[1]).get_exprs()
>> -except SystemExit:
>> - raise
>> +class QAPISchemaTestVisitor(QAPISchemaVisitor):
>> + def visit_enum_type(self, name, info, values):
>> + print 'enum %s %s' % (name, values)
>> + def visit_object_type(self, name, info, base, members, variants):
>> + print 'object %s' % name
>> + if base:
>> + print ' base %s' % base.name
>> + for m in members:
>> + print ' member %s: %s optional=%s' % (m.name, m.type.name,
>> + m.optional)
>> + self._print_variants(variants)
>
> So here is where information was missing when visiting a simple union.
> Why didn't 'kind' get injected into members? And...
Because members are the explicit members. A simple union's implicit tag
member is in QAPISchemaObjectTypeVariants. See further below.
>> + def visit_alternate_type(self, name, info, variants):
>> + print 'alternate %s' % name
>> + self._print_variants(variants)
>> + def visit_command(self, name, info, args, rets, gen, success_response):
>> + print 'command %s %s -> %s' % (name, (args and args.name),
>> + (rets and rets.name))
>> + print ' gen=%s success_response=%s' % (gen, success_response)
>> + def visit_event(self, name, info, data):
>> + print 'event %s %s' % (name, data and data.name)
>>
>> -pprint(exprs)
>> -pprint(enum_types)
>> -pprint(struct_types)
>> + @staticmethod
>> + def _print_variants(variants):
>> + if variants:
>> + if variants.tag_name:
>> + print ' tag %s' % variants.tag_name
>
> ...shouldn't a simple union report 'kind' as its tag_name?
It could. Here's why it currently isn't.
QAPISchemaObjectType has members .base and .local_members, which come
straight from the type definition.
These get flattened into .members.
We do create a simple union's implicit member 'kind', and store it in
.variants.tag_member. But we don't add it to .local_members, only to
.members, similar to how .base's members are only in .members.
We don't print .members here. Instead, we print .base, .local_members
and .variants. The part printing .variants doesn't print
.variants.tag_member.
We need to draw the line on what to print *somewhere*. Where exactly is
of course debatable. My working idea on what to print here is "print
everything that isn't derived from other stuff".
>> + for v in variants.variants:
>> + print ' case %s: %s flat=%s' % (v.name, v.type.name, v.flat)
>> +
>> +schema = QAPISchema(sys.argv[1])
>> +schema.visit(QAPISchemaTestVisitor())
>> diff --git a/tests/qapi-schema/type-bypass.out
>> b/tests/qapi-schema/type-bypass.out
>
> Overall, fairly slick, but I have enough questions about the
> representation of a simple union that I'll wait for the non-RFC respin
> to see if you change any design.
Thanks!
