[Top][All Lists]

[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: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v4 01/10] qapi: Assert in places where variants are not handled
Date: Tue, 08 Mar 2016 18:46:35 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Eric Blake <address@hidden> writes:

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]