diff mbox series

[v4,06/51] qapi: pass 'if' condition into QAPISchemaEntity objects

Message ID 20180111213250.16511-7-marcandre.lureau@redhat.com
State New
Headers show
Series Hi, | expand

Commit Message

Marc-André Lureau Jan. 11, 2018, 9:32 p.m. UTC 
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 wrapped type's 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
simpler and good enough for now.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi.py | 98 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 32 deletions(-)

Comments

Markus Armbruster Feb. 5, 2018, 6:16 a.m. UTC | #1 
Marc-André Lureau <marcandre.lureau@redhat.com> 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 wrapped type's generated for simple union variants,

Editing accident: you changed "is the wrapper types" to "is the wrapped
type's" here instead of

> 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

changing "use wrapped type's"  to "use the wrapped type's" here.  Can
touch up on commit.

> simpler and good enough for now.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

R-by stands.
Markus Armbruster Feb. 6, 2018, 10:12 a.m. UTC | #2 
Marc-André Lureau <marcandre.lureau@redhat.com> 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 wrapped type's 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
> simpler and good enough for now.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  scripts/qapi.py | 98 ++++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 66 insertions(+), 32 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 27df0fcf48..8f54dead8d 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -991,8 +991,17 @@ def check_exprs(exprs):
>  # Schema compiler frontend
>  #
>  
> +def listify_cond(ifcond):
> +    if not ifcond:
> +        return []
> +    elif not isinstance(ifcond, list):
> +        return [ifcond]
> +    else:
> +        return ifcond

pylint complains:

    R:995, 4: Unnecessary "else" after "return" (no-else-return)

Matter of taste.  Mine happens to agree with pylint's.  Suggest:

   def listify_cond(ifcond):
       if not ifcond:
           return []
       if not isinstance(ifcond, list):
           return [ifcond]
       return ifcond

> +
> +
>  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)
[...]
Marc-André Lureau Feb. 6, 2018, 11:06 a.m. UTC | #3 
Hi

On Tue, Feb 6, 2018 at 11:12 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Marc-André Lureau <marcandre.lureau@redhat.com> 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 wrapped type's 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
>> simpler and good enough for now.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  scripts/qapi.py | 98 ++++++++++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 66 insertions(+), 32 deletions(-)
>>
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 27df0fcf48..8f54dead8d 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -991,8 +991,17 @@ def check_exprs(exprs):
>>  # Schema compiler frontend
>>  #
>>
>> +def listify_cond(ifcond):
>> +    if not ifcond:
>> +        return []
>> +    elif not isinstance(ifcond, list):
>> +        return [ifcond]
>> +    else:
>> +        return ifcond
>
> pylint complains:
>
>     R:995, 4: Unnecessary "else" after "return" (no-else-return)
>
> Matter of taste.  Mine happens to agree with pylint's.  Suggest:
>
>    def listify_cond(ifcond):
>        if not ifcond:
>            return []
>        if not isinstance(ifcond, list):
>            return [ifcond]
>        return ifcond
>

There are so many errors with pylint, that I don't bother running it.
How did you notice? you run diff of error output between commits?

pycodestyle --diff is more convenient, and silent on this code.

Feel free to touch if you pick the patch.

>> +
>> +
>>  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)
> [...]
>
Markus Armbruster Feb. 6, 2018, 12:14 p.m. UTC | #4 
Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Tue, Feb 6, 2018 at 11:12 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Marc-André Lureau <marcandre.lureau@redhat.com> 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 wrapped type's 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
>>> simpler and good enough for now.
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  scripts/qapi.py | 98 ++++++++++++++++++++++++++++++++++++++-------------------
>>>  1 file changed, 66 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>>> index 27df0fcf48..8f54dead8d 100644
>>> --- a/scripts/qapi.py
>>> +++ b/scripts/qapi.py
>>> @@ -991,8 +991,17 @@ def check_exprs(exprs):
>>>  # Schema compiler frontend
>>>  #
>>>
>>> +def listify_cond(ifcond):
>>> +    if not ifcond:
>>> +        return []
>>> +    elif not isinstance(ifcond, list):
>>> +        return [ifcond]
>>> +    else:
>>> +        return ifcond
>>
>> pylint complains:
>>
>>     R:995, 4: Unnecessary "else" after "return" (no-else-return)
>>
>> Matter of taste.  Mine happens to agree with pylint's.  Suggest:
>>
>>    def listify_cond(ifcond):
>>        if not ifcond:
>>            return []
>>        if not isinstance(ifcond, list):
>>            return [ifcond]
>>        return ifcond
>>
>
> There are so many errors with pylint, that I don't bother running it.

pylint reports lots of stuff that is actually just fine.

> How did you notice? you run diff of error output between commits?

Yes.

> pycodestyle --diff is more convenient, and silent on this code.

Formerly known as pep8.  Confusingly, Fedora 26 packages both
separately.  Thanks for the pointer.

> Feel free to touch if you pick the patch.

Okay.

>>> +
>>> +
>>>  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)
>> [...]
>>
diff mbox series

Patch

diff --git a/scripts/qapi.py b/scripts/qapi.py
index 27df0fcf48..8f54dead8d 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -991,8 +991,17 @@  def check_exprs(exprs):
 # Schema compiler frontend
 #
 
+def listify_cond(ifcond):
+    if not ifcond:
+        return []
+    elif not isinstance(ifcond, list):
+        return [ifcond]
+    else:
+        return ifcond
+
+
 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)
@@ -1002,6 +1011,7 @@  class QAPISchemaEntity(object):
         # such place).
         self.info = info
         self.doc = doc
+        self.ifcond = listify_cond(ifcond)
 
     def c_name(self):
         return c_name(self.name)
@@ -1118,8 +1128,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)
@@ -1154,7 +1164,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
@@ -1162,6 +1172,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
@@ -1183,11 +1194,12 @@  class QAPISchemaArrayType(QAPISchemaType):
 
 
 class QAPISchemaObjectType(QAPISchemaType):
-    def __init__(self, name, info, doc, base, local_members, variants):
+    def __init__(self, name, info, doc, ifcond,
+                 base, local_members, variants):
         # struct has local_members, optional base, and no variants
         # flat union has base, variants, and no local_members
         # simple union has local_members, variants, and no base
-        QAPISchemaType.__init__(self, name, info, doc)
+        QAPISchemaType.__init__(self, name, info, doc, ifcond)
         assert base is None or isinstance(base, str)
         for m in local_members:
             assert isinstance(m, QAPISchemaObjectTypeMember)
@@ -1375,8 +1387,8 @@  class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember):
 
 
 class QAPISchemaAlternateType(QAPISchemaType):
-    def __init__(self, name, info, doc, variants):
-        QAPISchemaType.__init__(self, name, info, doc)
+    def __init__(self, name, info, doc, ifcond, variants):
+        QAPISchemaType.__init__(self, name, info, doc, ifcond)
         assert isinstance(variants, QAPISchemaObjectTypeVariants)
         assert variants.tag_member
         variants.set_owner(name)
@@ -1412,9 +1424,9 @@  class QAPISchemaAlternateType(QAPISchemaType):
 
 
 class QAPISchemaCommand(QAPISchemaEntity):
-    def __init__(self, name, info, doc, arg_type, ret_type,
+    def __init__(self, name, info, doc, ifcond, arg_type, ret_type,
                  gen, success_response, boxed):
-        QAPISchemaEntity.__init__(self, name, info, doc)
+        QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
         self._arg_type_name = arg_type
@@ -1451,8 +1463,8 @@  class QAPISchemaCommand(QAPISchemaEntity):
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
-    def __init__(self, name, info, doc, arg_type, boxed):
-        QAPISchemaEntity.__init__(self, name, info, doc)
+    def __init__(self, name, info, doc, ifcond, arg_type, boxed):
+        QAPISchemaEntity.__init__(self, name, info, doc, ifcond)
         assert not arg_type or isinstance(arg_type, str)
         self._arg_type_name = arg_type
         self.arg_type = None
@@ -1536,22 +1548,22 @@  class QAPISchema(object):
                   ('null',   'null',    'QNull' + pointer_suffix)]:
             self._def_builtin_type(*t)
         self.the_empty_object_type = QAPISchemaObjectType(
-            'q_empty', None, None, None, [], None)
+            'q_empty', None, None, None, None, [], None)
         self._def_entity(self.the_empty_object_type)
         qtype_values = self._make_enum_members(['none', 'qnull', 'qnum',
                                                 'qstring', 'qdict', 'qlist',
                                                 'qbool'])
-        self._def_entity(QAPISchemaEnumType('QType', None, None,
+        self._def_entity(QAPISchemaEnumType('QType', None, None, None,
                                             qtype_values, 'QTYPE'))
 
     def _make_enum_members(self, values):
         return [QAPISchemaMember(v) for v in values]
 
-    def _make_implicit_enum_type(self, name, info, values):
+    def _make_implicit_enum_type(self, name, info, ifcond, values):
         # See also QAPISchemaObjectTypeMember._pretty_owner()
         name = name + 'Kind'   # Use namespace reserved by add_name()
         self._def_entity(QAPISchemaEnumType(
-            name, info, None, self._make_enum_members(values), None))
+            name, info, None, ifcond, self._make_enum_members(values), None))
         return name
 
     def _make_array_type(self, element_type, info):
@@ -1560,22 +1572,37 @@  class QAPISchema(object):
             self._def_entity(QAPISchemaArrayType(name, info, element_type))
         return name
 
-    def _make_implicit_object_type(self, name, info, doc, role, members):
+    def _make_implicit_object_type(self, name, info, doc, ifcond,
+                                   role, members):
         if not members:
             return None
         # See also QAPISchemaObjectTypeMember._pretty_owner()
         name = 'q_obj_%s-%s' % (name, role)
-        if not self.lookup_entity(name, QAPISchemaObjectType):
-            self._def_entity(QAPISchemaObjectType(name, info, doc, None,
-                                                  members, None))
+        typ = self.lookup_entity(name, QAPISchemaObjectType)
+        if typ:
+            # The implicit object type has multiple users.  This can
+            # happen only for simple unions' implicit wrapper types.
+            # Its ifcond should be the disjunction of its user's
+            # ifconds.  Not implemented.  Instead, we always pass the
+            # wrapped type's ifcond, which is trivially the same for all
+            # users.  It's also necessary for the wrapper to compile.
+            # But it's not tight: the disjunction need not imply it.  We
+            # may end up compiling useless wrapper types.
+            # TODO kill simple unions or implement the disjunction
+            assert ifcond == typ.ifcond
+        else:
+            self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond,
+                                                  None, members, None))
         return name
 
     def _def_enum_type(self, expr, info, doc):
         name = expr['enum']
         data = expr['data']
         prefix = expr.get('prefix')
+        ifcond = expr.get('if')
         self._def_entity(QAPISchemaEnumType(
-            name, info, doc, self._make_enum_members(data), prefix))
+            name, info, doc, ifcond,
+            self._make_enum_members(data), prefix))
 
     def _make_member(self, name, typ, info):
         optional = False
@@ -1595,7 +1622,8 @@  class QAPISchema(object):
         name = expr['struct']
         base = expr.get('base')
         data = expr['data']
-        self._def_entity(QAPISchemaObjectType(name, info, doc, base,
+        ifcond = expr.get('if')
+        self._def_entity(QAPISchemaObjectType(name, info, doc, ifcond, base,
                                               self._make_members(data, info),
                                               None))
 
@@ -1607,18 +1635,21 @@  class QAPISchema(object):
             assert len(typ) == 1
             typ = self._make_array_type(typ[0], info)
         typ = self._make_implicit_object_type(
-            typ, info, None, 'wrapper', [self._make_member('data', typ, info)])
+            typ, info, None, self.lookup_type(typ).ifcond,
+            'wrapper', [self._make_member('data', typ, info)])
         return QAPISchemaObjectTypeVariant(case, typ)
 
     def _def_union_type(self, expr, info, doc):
         name = expr['union']
         data = expr['data']
         base = expr.get('base')
+        ifcond = expr.get('if')
         tag_name = expr.get('discriminator')
         tag_member = None
         if isinstance(base, dict):
-            base = (self._make_implicit_object_type(
-                name, info, doc, 'base', self._make_members(base, info)))
+            base = self._make_implicit_object_type(
+                name, info, doc, ifcond,
+                'base', self._make_members(base, info))
         if tag_name:
             variants = [self._make_variant(key, value)
                         for (key, value) in data.iteritems()]
@@ -1626,12 +1657,12 @@  class QAPISchema(object):
         else:
             variants = [self._make_simple_variant(key, value, info)
                         for (key, value) in data.iteritems()]
-            typ = self._make_implicit_enum_type(name, info,
+            typ = self._make_implicit_enum_type(name, info, ifcond,
                                                 [v.name for v in variants])
             tag_member = QAPISchemaObjectTypeMember('type', typ, False)
             members = [tag_member]
         self._def_entity(
-            QAPISchemaObjectType(name, info, doc, base, members,
+            QAPISchemaObjectType(name, info, doc, ifcond, base, members,
                                  QAPISchemaObjectTypeVariants(tag_name,
                                                               tag_member,
                                                               variants)))
@@ -1639,11 +1670,12 @@  class QAPISchema(object):
     def _def_alternate_type(self, expr, info, doc):
         name = expr['alternate']
         data = expr['data']
+        ifcond = expr.get('if')
         variants = [self._make_variant(key, value)
                     for (key, value) in data.iteritems()]
         tag_member = QAPISchemaObjectTypeMember('type', 'QType', False)
         self._def_entity(
-            QAPISchemaAlternateType(name, info, doc,
+            QAPISchemaAlternateType(name, info, doc, ifcond,
                                     QAPISchemaObjectTypeVariants(None,
                                                                  tag_member,
                                                                  variants)))
@@ -1655,23 +1687,25 @@  class QAPISchema(object):
         gen = expr.get('gen', True)
         success_response = expr.get('success-response', True)
         boxed = expr.get('boxed', False)
+        ifcond = expr.get('if')
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
-                name, info, doc, 'arg', self._make_members(data, info))
+                name, info, doc, ifcond, 'arg', self._make_members(data, info))
         if isinstance(rets, list):
             assert len(rets) == 1
             rets = self._make_array_type(rets[0], info)
-        self._def_entity(QAPISchemaCommand(name, info, doc, data, rets,
+        self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, data, rets,
                                            gen, success_response, boxed))
 
     def _def_event(self, expr, info, doc):
         name = expr['event']
         data = expr.get('data')
         boxed = expr.get('boxed', False)
+        ifcond = expr.get('if')
         if isinstance(data, OrderedDict):
             data = self._make_implicit_object_type(
-                name, info, doc, 'arg', self._make_members(data, info))
-        self._def_entity(QAPISchemaEvent(name, info, doc, data, boxed))
+                name, info, doc, ifcond, 'arg', self._make_members(data, info))
+        self._def_entity(QAPISchemaEvent(name, info, doc, ifcond, data, boxed))
 
     def _def_exprs(self):
         for expr_elem in self.exprs: