[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 06/50] qapi: pass 'if' condition into QAPISch
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3 06/50] qapi: pass 'if' condition into QAPISchemaEntity objects |
Date: |
Wed, 06 Dec 2017 18:13:39 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.3 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> Built-in objects remain unconditional. Explicitly defined objects
> use the condition specified in the schema. Implicitly defined
> objects inherit their condition from their users. For most of them,
> there is exactly one user, so the condition to use is obvious. The
> exception is the wrapper types generated for simple union variants,
> which can be shared by any number of simple unions. The tight
> condition would be the disjunction of the conditions of these simple
> unions. For now, use wrapped type's condition instead. Much
the wrapped type's
> simpler and good enough for now.
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
> scripts/qapi.py | 89
> ++++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 57 insertions(+), 32 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 20c1abf915..0f55caa18d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -1000,7 +1000,7 @@ def check_exprs(exprs):
> #
>
> class QAPISchemaEntity(object):
> - def __init__(self, name, info, doc):
> + def __init__(self, name, info, doc, ifcond=None):
> assert isinstance(name, str)
> self.name = name
> # For explicitly defined entities, info points to the (explicit)
> @@ -1010,6 +1010,7 @@ class QAPISchemaEntity(object):
> # such place).
> self.info = info
> self.doc = doc
> + self.ifcond = ifcond
>
> def c_name(self):
> return c_name(self.name)
> @@ -1126,8 +1127,8 @@ class QAPISchemaBuiltinType(QAPISchemaType):
>
>
> class QAPISchemaEnumType(QAPISchemaType):
> - def __init__(self, name, info, doc, values, prefix):
> - QAPISchemaType.__init__(self, name, info, doc)
> + def __init__(self, name, info, doc, ifcond, values, prefix):
> + QAPISchemaType.__init__(self, name, info, doc, ifcond)
> for v in values:
> assert isinstance(v, QAPISchemaMember)
> v.set_owner(name)
> @@ -1162,7 +1163,7 @@ class QAPISchemaEnumType(QAPISchemaType):
>
> class QAPISchemaArrayType(QAPISchemaType):
> def __init__(self, name, info, element_type):
> - QAPISchemaType.__init__(self, name, info, None)
> + QAPISchemaType.__init__(self, name, info, None, None)
> assert isinstance(element_type, str)
> self._element_type_name = element_type
> self.element_type = None
> @@ -1170,6 +1171,7 @@ class QAPISchemaArrayType(QAPISchemaType):
> def check(self, schema):
> self.element_type = schema.lookup_type(self._element_type_name)
> assert self.element_type
> + self.ifcond = self.element_type.ifcond
>
> def is_implicit(self):
> return True
In my review of v2, I wrote:
This is subtler than it looks on first glance.
All the other entities set self.ifcond in their constructor to the true
value passed in as argument.
QAPISchemaArrayType doesn't take such an argument. Instead, it inherits
its .ifcond from its .element_type. However, .element_type isn't known
at construction time if it's a forward reference. We therefore delay
setting it until .check() time. You do the same for .ifcond (no
choice).
Before .check(), .ifcond is None, because the constructor sets it that
way: it calls QAPISchemaType.__init__() without passing a ifcond
argument, which then sets self.ifcond to its default argument None.
Pitfall: accessing ent.ifcond before ent.check() works *except* when ent
is an array type. Hmm.
The problem is of course more general. We commonly initialize
attributes to None in .init(), then set their real value in .check().
Accessing the attribute before .check() yields None. If we're lucky,
the code that accesses the attribute prematurely chokes on None.
It won't for .ifcond, because None is a legitimate value.
Perhaps we should leave such attributes undefined until .check(). What
do you think? No need to tie that idea to this series, though.
[...]
With the commit message tidied up:
Reviewed-by: Markus Armbruster <address@hidden>
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH v3 06/50] qapi: pass 'if' condition into QAPISchemaEntity objects,
Markus Armbruster <=