[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 19/27] qapi: Check for qapi collisions of fla
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v9 19/27] qapi: Check for qapi collisions of flat union branches |
Date: |
Thu, 05 Nov 2015 08:59:31 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
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.
- [Qemu-devel] [PATCH v9 22/27] qapi: Remove outdated tests related to QMP/branch collisions, (continued)
- [Qemu-devel] [PATCH v9 22/27] qapi: Remove outdated tests related to QMP/branch collisions, Eric Blake, 2015/11/04
- [Qemu-devel] [PATCH v9 26/27] qapi: Remove dead visitor code, Eric Blake, 2015/11/04
- [Qemu-devel] [PATCH v9 25/27] qapi: Add positive tests to qapi-schema-test, Eric Blake, 2015/11/04
- [Qemu-devel] [PATCH v9 12/27] qapi-types: Simplify gen_struct_field[s], Eric Blake, 2015/11/04
- [Qemu-devel] [PATCH v9 19/27] qapi: Check for qapi collisions of flat union branches, Eric Blake, 2015/11/04
[Qemu-devel] [PATCH v9 04/27] qapi: Simplify error testing in test-qmp-*, Eric Blake, 2015/11/04
[Qemu-devel] [PATCH v9 21/27] qapi: Factor out QAPISchemaObjectType.check_clash(), Eric Blake, 2015/11/04