[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 01/10] qapi: Assert in places where variants
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v4 01/10] qapi: Assert in places where variants are not handled |
Date: |
Tue, 8 Mar 2016 08:49:52 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
On 03/08/2016 03:12 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
>
>> We are getting closer to the point where we could use one union
>> as the base or variant type within another union type (as long
>> as there are no collisions between any possible combination of
>> member names allowed across all discriminator choices). But
>> until we get to that point, it is worth asserting that variants
>> are not present in places where we are not prepared to handle
>> them: base types must still be plain structs, and anywhere we
>> explode a struct into a parameter list (events and command
>> marshalling), we don't support variants in that explosion.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>
>> +++ b/scripts/qapi.py
>> @@ -960,6 +960,7 @@ class QAPISchemaObjectType(QAPISchemaType):
>> assert isinstance(self.base, QAPISchemaObjectType)
>> self.base.check(schema)
>> self.base.check_clash(schema, self.info, seen)
>> + assert not self.base.variants
>
> I'd move this two lines up, so it's next to the isinstance.
>
> Assertions in .check() are place-holders for semantic checks that
> haven't been moved from the old semantic analysis to the classes.
> Whenever we add one, we should double-check the old semantic analysis
> catches whatever we assert. For object types, that's check_struct() and
> check_union(). Both check_type() the base with allow_metas=['struct']),
> so we're good.
>
> Inconsistency: you add the check for base, but not for variants.
>
> On closer look, adding it for either is actually redundant, because
> se.f.base.check_clash() already asserts it, with a nice "not
> implemented" comment.
>
> If we think asserting twice is useful for base, then it's useful for
> variants, too. But I think asserting once suffices.
So basically, we can drop this hunk, right?
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v4 03/10] qapi: Make c_type() more OO-like, Eric Blake, 2016/03/05
[Qemu-devel] [PATCH v4 02/10] qapi: Fix command with named empty argument type, Eric Blake, 2016/03/05
[Qemu-devel] [PATCH v4 08/10] qapi: Allow anonymous base for flat union, Eric Blake, 2016/03/05
[Qemu-devel] [PATCH v4 10/10] qapi: Populate info['name'] for each entity, Eric Blake, 2016/03/05