Re: [Qemu-devel] [PATCH v8 13/18] qapi: Track owner of each object member

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4230 bytes --]

On 10/13/2015 07:14 AM, Markus Armbruster wrote:
> Eric Blake <eblake@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 describe(self):
>> +        owner = self.owner
>> +        # See QAPISchema._make_implicit_object_type() - reverse the
>> +        # mapping there to create a nice human-readable description
>> +        if owner.startswith(':obj-'):
>> +            owner = owner[5:]
>> +            if owner.endswith('-arg'):
>> +                source = '(argument of %s)' % owner[:4]
>> +            elif owner.endswith('-data'):
>> +                source = '(data of %s)' % owner[:5]
>> +            else:
>> +                assert owner.endswith('-wrapper')
>> +                source = '(branch of %s)' % owner[:8]
>> +        else:
>> +            source = '(%s of %s)' % (self.role, owner)
>> +        return "'%s' %s" % (self.name, source)
> 
> Perhaps not exactly pretty, but it gets the job done with minimal fuss.
> Sold.

Ouch - I think these should be owner[:-4] (and similar) - I want the
slice that excludes the last four bytes, and not one that is four bytes
long.  (It happened to work in my tests because I was testing on
:obj-oops-arg, where owner[:4] and owner[:-4] gave the same 'oops' string).

It's an easy fixup to squash in, so I'm not yet sure if I need a full v9
respin.

> 
>> +
>> +    role = 'member'
>> +
>>
> 
> Class variables are usually defined first, before methods.

Sure, can do. pep8 didn't complain, and I didn't check if pylint warns
about it.

> 
>>  # TODO Drop this class once we no longer have the 'type'/'kind' mismatch
>>  class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>> @@ -1034,6 +1061,11 @@ class QAPISchemaObjectTypeUnionTag(QAPISchemaObjectTypeMember):
>>          assert self.type.is_implicit()
>>          return 'kind'
>>
>> +    def describe(self):
>> +        # Must override superclass describe() because self.name is 'type'
> 
> I don't find this argument convincing.  I think 'kind' is exactly as
> unhelpful as 'type' is.  Specifically, should we report a QMP clash,
> calling it 'kind' is confusing (it actually clashes with 'type').
> Conversely 'type' is confusing when we report a C clash.
> 
> The helpful part...
> 
>> +        assert self.owner[0] != ':'
>> +        return "'kind' (implicit tag of %s)" % self.owner
>> +
> 
> ... is (implicit tag of FOO).
> 
> Fortunately, you're working towards killing the kind vs. type confusion.
> Suggest to either rephrase the comment, or simply drop it.

Drop the comment is fine by me.

I personally thought the message needed to report 'kind', because 16/18
ends up reporting:

+tests/qapi-schema/union-clash-type.json:8: 'kind' (branch of TestUnion)
collides with 'kind' (implicit tag of TestUnion)

If I didn't override describe() at all, it would report:

...: 'kind' (branch of TestUnion) collides with 'type' (member of TestUnion)

If all I did was override 'role = "implicit tag"' instead of describe(),
it would report:

...: 'kind' (branch of TestUnion) collides with 'type' (implicit tag of
TestUnion)

Of course when a later subset fixes the kind/type misnomer, the error
message changes:

...: 'type' (branch of TestUnion) collides with 'type' (member of TestUnion)

where we lose the "implicit tag" because we completely lose the subclass.

Any preferences on which error message to prefer, and therefore how much
(or how little) we need to override in this temporary subclass?  Or is
this an argument for making the subclass permanent (with a 'role =
"implicit tag"') even after the kind/type rename?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]