Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check()

[Eric Blake <eblake@redhat.com>]

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

On 10/12/2015 10:22 AM, Eric Blake wrote:

> 
>>> +    def check(self, schema, info, tag_type, seen, flat):
>>>          QAPISchemaObjectTypeMember.check(self, schema, info, [], seen)

seen gets modified here...

>>>          assert self.name in tag_type.values
>>> +        if flat:
>>> +            # For flat unions, check that each QMP member does not
>>> +            # collide with any non-variant members. No type can
>>> +            # contain itself as a flat variant
>>> +            self.type.check(schema)
>>> +            assert not self.type.variants    # not implemented
>>> +            for m in self.type.members:
>>> +                if m.c_name() in seen:

...and it is the modified seen here that causes...

>>  union-clash-data.json:6: Member 'data' of branch 'data' collides with 'data' (branch of TestUnion)

...the incorrect error.

>>
>> The FIXME in union-clash-data.json actually asks for an error, because
>> 'data' clashes with our stupid filler to avoid empty unions.  But that's
>> not what this error message reports!  I think what happens is this:
>> QAPISchemaObjectTypeVariant.check() adds the case name self.name (here:
>> 'data') to seen.  When we then check the case's 'data': 'int' member, it
>> finds the with the case name, and reports a clash.  I can't see why it
>> should.
>>
>> This clashing business is awfully confusing, because we have to consider
>> (flat unions, simple unions, alternates) times (QMP, C).
>>
>> It would be simpler if we could make C clash only when QMP clashes.
> 
> If a QMP clash always caused a C clash, life would be a bit simpler. We
> aren't going to get there (because in a flat union, hiding the members
> of the branch type behind a single pointer means those members don't
> clash with any C names of the overall union), but I can certainly try to
> make the comments explain what is going on.

I've managed to fix it with a dict(seen) and some comments, and will
post a v8.  Great catch, by the way.

> 
>>
>> We might want to separate out alternates.  Dunno.
> 
> And to some extent, subset C already does some of that separation
> (because I try to convert from 'FooKind type' to 'qtype_code type'
> without even creating an implicit 'FooKind' enum).  It sounds like
> you're okay with any well-documented TODO warts related to alternates
> for the purposes of this subset B, especially if I can then clean those
> warts back up in subset C.  But what v8 of subset B needs to avoid is
> any warts based on simple vs. flat unions.  I can live with that.
> 
> I'm still trying to figure out if hoisting the kind=>type rename into
> subset B makes life easier or harder (it certainly causes me more rebase
> churn, but that's not necessarily a bad thing).

At this point, it was easier to live with a temporary hack for
QAPISchemaObjectTypeUnionTag than it was to rebase my kind=>type rename
patch to be this early in the overall series. We'll get there, but I
agree with the approach we've been taking of one subset at a time.

-- 
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 --]