[Qemu-devel] [PATCH v5 10/28] qapi: Segregate anonymous unions into alternates in generator

[Eric Blake <eblake@redhat.com>]

Special-casing 'discriminator == {}' for handling anonymous unions
is getting awkward; since this particular type is not always a
dictionary on the wire, it is easier to treat it as a completely
different class of type, "alternate", so that if a type is listed
in the union_types array, we know it is not an anonymous union.

This patch just further segregates union handling, to make sure that
anonymous unions are not stored in union_types, and splitting up
check_union() into separate functions.  A future patch will change
the qapi grammar, and having the segregation already in place will
make it easier to deal with the distinct meta-type.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi-types.py |  6 ++--
 scripts/qapi-visit.py |  4 +--
 scripts/qapi.py       | 94 +++++++++++++++++++++++++++++----------------------
 3 files changed, 58 insertions(+), 46 deletions(-)

diff --git a/scripts/qapi-types.py b/scripts/qapi-types.py
index 2390887..c9e0201 100644
--- a/scripts/qapi-types.py
+++ b/scripts/qapi-types.py
@@ -170,7 +170,7 @@ typedef enum %(name)s

     return lookup_decl + enum_decl

-def generate_anon_union_qtypes(expr):
+def generate_alternate_qtypes(expr):

     name = expr['union']
     members = expr['data']
@@ -181,7 +181,7 @@ const int %(name)s_qtypes[QTYPE_MAX] = {
     name=name)

     for key in members:
-        qtype = find_anonymous_member_qtype(members[key])
+        qtype = find_alternate_member_qtype(members[key])
         assert qtype, "Invalid anonymous union member"

         ret += mcgen('''
@@ -408,7 +408,7 @@ for expr in exprs:
             fdef.write(generate_enum_lookup('%sKind' % expr['union'],
                                             expr['data'].keys()))
         if expr.get('discriminator') == {}:
-            fdef.write(generate_anon_union_qtypes(expr))
+            fdef.write(generate_alternate_qtypes(expr))
     else:
         continue
     fdecl.write(ret)
diff --git a/scripts/qapi-visit.py b/scripts/qapi-visit.py
index 3f82bd4..77b0a1f 100644
--- a/scripts/qapi-visit.py
+++ b/scripts/qapi-visit.py
@@ -237,7 +237,7 @@ void visit_type_%(name)s(Visitor *m, %(name)s *obj, const char *name, Error **er
 ''',
                  name=name)

-def generate_visit_anon_union(name, members):
+def generate_visit_alternate(name, members):
     ret = mcgen('''

 void visit_type_%(name)s(Visitor *m, %(name)s **obj, const char *name, Error **errp)
@@ -302,7 +302,7 @@ def generate_visit_union(expr):

     if discriminator == {}:
         assert not base
-        return generate_visit_anon_union(name, members)
+        return generate_visit_alternate(name, members)

     enum_define = discriminator_find_enum_define(expr)
     if enum_define:
diff --git a/scripts/qapi.py b/scripts/qapi.py
index 39cc88b..17252e9 100644
--- a/scripts/qapi.py
+++ b/scripts/qapi.py
@@ -224,21 +224,16 @@ def find_base_fields(base):
         return None
     return base_struct_define['data']

-# Return the qtype of an anonymous union branch, or None on error.
-def find_anonymous_member_qtype(qapi_type):
+# Return the qtype of an alternate branch, or None on error.
+def find_alternate_member_qtype(qapi_type):
     if builtin_types.has_key(qapi_type):
         return builtin_types[qapi_type]
     elif find_struct(qapi_type):
         return "QTYPE_QDICT"
     elif find_enum(qapi_type):
         return "QTYPE_QSTRING"
-    else:
-        union = find_union(qapi_type)
-        if union:
-            discriminator = union.get('discriminator')
-            if discriminator == {}:
-                return None
-            return "QTYPE_QDICT"
+    elif find_union(qapi_type):
+        return "QTYPE_QDICT"
     return None

 # Return the discriminator enum define if discriminator is specified as an
@@ -276,22 +271,13 @@ def check_union(expr, expr_info):
     discriminator = expr.get('discriminator')
     members = expr['data']
     values = { 'MAX': '(automatic)' }
-    types_seen = {}

-    # Three types of unions, determined by discriminator.
+    # Two types of unions, determined by discriminator.
+    assert discriminator != {}

-    # If the value of member 'discriminator' is {}, it's an
-    # anonymous union, and must not have a base.
-    if discriminator == {}:
-        enum_define = None
-        if base:
-            raise QAPIExprError(expr_info,
-                                "Anonymous union '%s' must not have a base"
-                                % name)
-
-    # Else if the union object has no member 'discriminator', it's an
+    # If the union object has no member 'discriminator', it's an
     # ordinary union.  For now, it must not have a base.
-    elif not discriminator:
+    if not discriminator:
         enum_define = None
         if base:
             raise QAPIExprError(expr_info,
@@ -346,24 +332,46 @@ def check_union(expr, expr_info):
                                     % (name, key, values[c_key]))
             values[c_key] = key

-        # Ensure anonymous unions have no type conflicts.
-        if discriminator == {}:
-            if isinstance(value, list):
-                raise QAPIExprError(expr_info,
-                                    "Anonymous union '%s' member '%s' must "
-                                    "not be array type" % (name, key))
-            qtype = find_anonymous_member_qtype(value)
-            if not qtype:
-                raise QAPIExprError(expr_info,
-                                    "Anonymous union '%s' member '%s' has "
-                                    "invalid type '%s'" % (name, key, value))
-            if qtype in types_seen:
-                raise QAPIExprError(expr_info,
-                                    "Anonymous union '%s' member '%s' has "
-                                    "same QObject type as member '%s'"
-                                    % (name, key, types_seen[qtype]))
-            types_seen[qtype] = key
+def check_alternate(expr, expr_info):
+    name = expr['union']
+    base = expr.get('base')
+    discriminator = expr.get('discriminator')
+    members = expr['data']
+    values = { 'MAX': '(automatic)' }
+    types_seen = {}

+    assert discriminator == {}
+    if base:
+        raise QAPIExprError(expr_info,
+                            "Anonymous union '%s' must not have a base"
+                            % name)
+
+    # Check every branch
+    for (key, value) in members.items():
+        # Check for conflicts in the generated enum
+        c_key = _generate_enum_string(key)
+        if c_key in values:
+            raise QAPIExprError(expr_info,
+                                "Union '%s' member '%s' clashes with '%s'"
+                                % (name, key, values[c_key]))
+        values[c_key] = key
+
+        # Ensure alternates have no type conflicts.
+        if isinstance(value, list):
+            raise QAPIExprError(expr_info,
+                                "Anonymous union '%s' member '%s' must "
+                                "not be array type" % (name, key))
+        qtype = find_alternate_member_qtype(value)
+        if not qtype:
+            raise QAPIExprError(expr_info,
+                                "Anonymous union '%s' member '%s' has "
+                                "invalid type '%s'" % (name, key, value))
+        if qtype in types_seen:
+            raise QAPIExprError(expr_info,
+                                "Anonymous union '%s' member '%s' has "
+                                "same QObject type as member '%s'"
+                                % (name, key, types_seen[qtype]))
+        types_seen[qtype] = key

 def check_enum(expr, expr_info):
     name = expr['enum']
@@ -393,7 +401,10 @@ def check_exprs(schema):
         if expr.has_key('enum'):
             check_enum(expr, info)
         elif expr.has_key('union'):
-            check_union(expr, info)
+            if expr.get('discriminator') == {}:
+                check_alternate(expr, info)
+            else:
+                check_union(expr, info)
         elif expr.has_key('event'):
             check_event(expr, info)

@@ -535,7 +546,8 @@ def find_struct(name):

 def add_union(definition):
     global union_types
-    union_types.append(definition)
+    if definition.get('discriminator') != {}:
+        union_types.append(definition)

 def find_union(name):
     global union_types
-- 
2.1.0