diff mbox

[RFC,v3,02/32] qapi: New QAPISchema intermediate reperesentation

Message ID 87y4gre3d7.fsf@blackfin.pond.sub.org
State New
Headers show

Commit Message

Markus Armbruster Aug. 31, 2015, 6:09 p.m. UTC 
Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
>
>> On 08/05/2015 12:23 AM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>> 
>>>> On 08/04/2015 09:57 AM, Markus Armbruster wrote:
>>>>> The QAPI code generators work with a syntax tree (nested dictionaries)
>>>>> plus a few symbol tables (also dictionaries) on the side.
>>>>>
>>
>>>>> +class QAPISchemaArrayType(QAPISchemaType):
>>>>> +    def __init__(self, name, info, element_type):
>>>>> +        QAPISchemaType.__init__(self, name, info)
>>>>> +        assert isinstance(element_type, str)
>>>>> +        self.element_type_name = element_type
>>>>> +        self.element_type = None
>>>>> +    def check(self, schema):
>>>>> +        self.element_type = schema.lookup_type(self.element_type_name)
>>>>> +        assert self.element_type
>>>>
>>>> Is it worth adding:
>>>>
>>>> assert not isinstance(self.element_type, QAPISchemaArrayType)
>>>>
>>>> since we don't allow 2D arrays?
>>> 
>>> If the generators actually rely on it, yes.
>>
>> Hmm. What happens if you do
>>  { 'command': 'Foo', 'returns': [ 'intList' ] }
>>
>>> 
>>> If it's just an arbitrary schema language restriction, probably no.
>>
>> That's a tough judgment call. We don't currently allow [ [ 'int' ] ],
>> and the [ 'intList' ] hack is gross. On the other hand, I'm having a
>> tough time coming up with technical reasons why we can't do it (arrays
>> as a parameter or return type should work, and 2D arrays just add
>> another layer of '*' to the C code).
>
> Perhaps a quick experiment can decide the nature of the restriction.

The appended experimental frontend patch passes "make check".  Looks
like the backends are just fine with arrays of arrays.  I'll therefore
refrain from adding "element type isn't array" assertions to backends.

Since there's plenty of QAPI work on list already, I'll shelve this
patch for now.  We can revisit nested arrays later.

Comments

Eric Blake Sept. 3, 2015, 2:52 p.m. UTC | #1 
On 08/31/2015 12:09 PM, Markus Armbruster wrote:

>>>>>
>>>>> since we don't allow 2D arrays?
>>>>
>>>> If the generators actually rely on it, yes.

> The appended experimental frontend patch passes "make check".  Looks
> like the backends are just fine with arrays of arrays.  I'll therefore
> refrain from adding "element type isn't array" assertions to backends.
> 
> Since there's plenty of QAPI work on list already, I'll shelve this
> patch for now.  We can revisit nested arrays later.

Another thing to think about if we allow nested arrays: patch 31/32 maps
'uint8' to 'int', and ['uint8'] to ['int'], but leaves [['uint8']]
untouched.  We'd have to add a while loop in qapi-introspect.py to
ensure we drill down through all levels of an array.
Markus Armbruster Sept. 3, 2015, 4:13 p.m. UTC | #2 
Eric Blake <eblake@redhat.com> writes:

> On 08/31/2015 12:09 PM, Markus Armbruster wrote:
>
>>>>>>
>>>>>> since we don't allow 2D arrays?
>>>>>
>>>>> If the generators actually rely on it, yes.
>
>> The appended experimental frontend patch passes "make check".  Looks
>> like the backends are just fine with arrays of arrays.  I'll therefore
>> refrain from adding "element type isn't array" assertions to backends.
>> 
>> Since there's plenty of QAPI work on list already, I'll shelve this
>> patch for now.  We can revisit nested arrays later.
>
> Another thing to think about if we allow nested arrays: patch 31/32 maps
> 'uint8' to 'int', and ['uint8'] to ['int'], but leaves [['uint8']]
> untouched.  We'd have to add a while loop in qapi-introspect.py to
> ensure we drill down through all levels of an array.

Yes, that's broken in my experimental patch.
diff mbox

Patch

diff --git a/scripts/qapi.py b/scripts/qapi.py
index d1def74..ba51a03 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -434,11 +434,11 @@  def check_type(expr_info, source, value, allow_array = False,
         return
 
     # Check if array type for value is okay
-    if isinstance(value, list):
+    while isinstance(value, list):
         if not allow_array:
             raise QAPIExprError(expr_info,
                                 "%s cannot be an array" % source)
-        if len(value) != 1 or not isinstance(value[0], str):
+        if len(value) != 1 or not (isinstance(value[0], str) or isinstance(value[0], list)):
             raise QAPIExprError(expr_info,
                                 "%s: array type must contain single type name"
                                 % source)
@@ -1056,6 +1056,9 @@  class QAPISchema(object):
         return name
 
     def _make_array_type(self, element_type):
+        if isinstance(element_type, list):
+            assert len(element_type) == 1
+            element_type = self._make_array_type(element_type[0])
         name = element_type + 'List'
         if not self.lookup_type(name):
             self._def_entity(QAPISchemaArrayType(name, None, element_type))
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index e855018..8072026 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -74,6 +74,7 @@ 
             'boolean': ['bool'],
             'string': ['str'],
             'sizes': ['size'],
+            'array': [['str']],
             'any': ['any'] } }
 
 # testing commands
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index a9c87a0..c467d47 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -37,6 +37,8 @@  object :obj-str-wrapper
     member data: str optional=False
 object :obj-strList-wrapper
     member data: strList optional=False
+object :obj-strListList-wrapper
+    member data: strListList optional=False
 object :obj-uint16List-wrapper
     member data: uint16List optional=False
 object :obj-uint32List-wrapper
@@ -105,8 +107,9 @@  object UserDefNativeListUnion
     case boolean: :obj-boolList-wrapper
     case string: :obj-strList-wrapper
     case sizes: :obj-sizeList-wrapper
+    case array: :obj-strListList-wrapper
     case any: :obj-anyList-wrapper
-enum UserDefNativeListUnionKind ['integer', 's8', 's16', 's32', 's64', 'u8', 'u16', 'u32', 'u64', 'number', 'boolean', 'string', 'sizes', 'any']
+enum UserDefNativeListUnionKind ['integer', 's8', 's16', 's32', 's64', 'u8', 'u16', 'u32', 'u64', 'number', 'boolean', 'string', 'sizes', 'array', 'any']
 object UserDefOne
     base UserDefZero
     member string: str optional=False