Re: [Qemu-devel] [PATCH v9 19/27] qapi: Check for qapi collisions of flat union branches

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 11/04/2015 04:11 PM, Eric Blake wrote:
>> On 11/04/2015 12:01 PM, Markus Armbruster wrote:
>>> Eric Blake <address@hidden> writes:
>>>
>>>> Right now, our ad hoc parser ensures that we cannot have a
>>>> flat union that introduces any qapi member names that would
>>>> conflict with the non-variant qapi members already present
>>>> from the union's base type (see flat-union-clash-member.json).
>>>> We want QAPISchema*.check() to make the same check, so we can
>>>> later reduce some of the ad hoc checks.
>>>>
>> 
>
>>> Why can't we simply add the new code to QAPISchemaObjectType.check()?
>> 
>> We could, but we'd need a nested for-loop:
>> 
>> for v in variants.variants:
>>     v.type.check(schema)
>>     assert not v.type.variants
>>     vseen = dict(seen)
>>     for m in v.type.members:
>>         m.check_clash(seen)

Looks clear enough to me.

>>> The rest of the clash checking is already there...
>> 
>> You do have a point there.  I guess it wouldn't be the end of the world
>> to have the loop nesting be explicit rather than indirect through the
>> intermediate Variants.check().

Hiding loop nesting behind method calls doesn't make code simpler :)

Methods help when they're useful abstractions.  For instance, hiding the
details of checking a member m behind m.check() is good.  But here,
we're checking relations between members.

>> I'll play with it; and depending on what I do, that may mean I don't
>> have to munge your other patches quite so heavily.
>
> And of course, as soon as I hit send, I had another thought - your
> patches already added Member.check_clash() called separately from the
> .check() chain; maybe I am better off adding a Variants.check_clash()
> separate from the .check() chain, rather than trying to cram the whole
> nested loop directly in ObjectType.check().

Matter of taste, can't tell without trying, use your judgement.