On 11/09/2015 07:26 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> Future commits will migrate semantic checking away from parsing >> and over to the various QAPISchema*.check() methods. But to >> report an error message about an incorrect semantic use of a >> member of an object type, it helps to know which type, command, >> or event owns the member. In particular, when a member is >> inherited from a base type, it is desirable to associate the >> member name with the base type (and not the type calling >> member.check()). >>
>>
>> + def _pretty_owner(self):
>> + # See QAPISchema._make_implicit_object_type() - reverse the
>> + # mapping there to create a nice human-readable description
>
> This comment confused me briefly. It applies to the following
> conditionals if-part, but not its else-part. Move it into the if-part?
Done.
>> @@ -1082,9 +1117,11 @@ class QAPISchemaAlternateType(QAPISchemaType):
>> QAPISchemaType.__init__(self, name, info)
>> assert isinstance(variants, QAPISchemaObjectTypeVariants)
>> assert not variants.tag_name
>> + variants.set_owner(name)
>> self.variants = variants
>>
>> def check(self, schema):
>> + self.variants.tag_member.set_owner(self.name)
>> self.variants.tag_member.check(schema)
>> self.variants.check(schema, {})
>>
>
> Odd: all other .set_owner() calls are in .__init__() or .set_owner().
> Can we move this one to __init__() for consistency?
Yes, done.
>
> I think we can: __init__() requires its variants argument to have a
> tag_member (it even asserts not variants.tag_name).
>
>> @@ -1212,6 +1249,7 @@ class QAPISchema(object):
>> def _make_implicit_object_type(self, name, info, role, members):
>> if not members:
>> return None
>> + # See also QAPISchemaObjectTypeMember.describe()
>
> Should this point to ._pretty_owner() instead?
Good call.
>
>> name = ':obj-%s-%s' % (name, role)
>> if not self.lookup_entity(name, QAPISchemaObjectType):
>> self._def_entity(QAPISchemaObjectType(name, info, None,
>> @@ -1315,7 +1353,7 @@ class QAPISchema(object):
>> data = expr.get('data')
>> if isinstance(data, OrderedDict):
>> data = self._make_implicit_object_type(
>> - name, info, 'arg', self._make_members(data, info))
>> + name, info, 'data', self._make_members(data, info))
>
> This is necessary only to make .pretty_owner() say 'data of EVENT_NAME'
> instead of 'argument of EVENT_NAME'. Do we really need different
> wording for commands and events?
>
> I'd make it say 'parameter of' for both commands and events. I
> habitually use "parameter" for formal parameters, and "argument" for
> actual arguments. Guess I've worked with the C standard too much...
'parameter' sounds better for both, at which point the messages no
longer differ,...
>
> For what it's worth, the QAPI schema language uses 'data' for both (and
> many other things, too), and the introspection schema uses 'arg-type'
> for both.
>
>> self._def_entity(QAPISchemaEvent(name, info, data))
>>
>> def _def_exprs(self):
>> diff --git a/tests/qapi-schema/qapi-schema-test.out
>> b/tests/qapi-schema/qapi-schema-test.out
>> index 786024e..f78ef04 100644
>> --- a/tests/qapi-schema/qapi-schema-test.out
>> +++ b/tests/qapi-schema/qapi-schema-test.out
>> @@ -1,9 +1,9 @@
>> object :empty
>> -object :obj-EVENT_C-arg
>> +object :obj-EVENT_C-data
>> member a: int optional=True
>> member b: UserDefOne optional=True
>> member c: str optional=False
>> -object :obj-EVENT_D-arg
>> +object :obj-EVENT_D-data
...so I no longer need to rename the implicit struct for events to allow
the differentiation. v11 is thus a smaller patch :)
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
