[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v7 11/14] qapi: Move duplicate member checks to schema check() |
Date: |
Mon, 12 Oct 2015 22:10:46 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v7 03/14] qapi: Drop redundant alternate-good test, (continued)
[Qemu-devel] [PATCH v7 10/14] qapi: Detect collisions in C member names, Eric Blake, 2015/10/08
[Qemu-devel] [PATCH v7 14/14] qapi: Detect base class loops, Eric Blake, 2015/10/08