qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] qapi: Refactor QAPISchemaVariants
@ 2024-03-20  7:43 Markus Armbruster
  2024-03-20  7:43 ` [PATCH 1/7] qapi: New QAPISchemaBranches, QAPISchemaAlternatives Markus Armbruster
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Markus Armbruster @ 2024-03-20  7:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: jsnow, peter.maydell, michael.roth

QAPISchemaVariants represents either a union type's branches, or an
alternate type's alternatives.  Much of its code is conditional on
which one it actually is.

This series moves the conditional code to new subtypes
QAPISchemaBranches and QAPISchemaAlternatives.

This also lets us treat QAPISchemaVariants.tag_member like the other
attribute that become known only in .check().

Markus Armbruster (7):
  qapi: New QAPISchemaBranches, QAPISchemaAlternatives
  qapi: Rename visitor parameter @variants to @branches
  qapi: Rename visitor parameter @variants to @alternatives
  qapi: Rename QAPISchemaObjectType.variants to .branches
  qapi: Rename QAPISchemaAlternateType.variants to .alternatives
  qapi: Move conditional code from QAPISchemaVariants to its subtypes
  qapi: Simplify QAPISchemaVariants @tag_member

 docs/sphinx/qapidoc.py         |  21 ++--
 scripts/qapi/commands.py       |   2 +-
 scripts/qapi/events.py         |   2 +-
 scripts/qapi/gen.py            |   2 +-
 scripts/qapi/introspect.py     |  15 +--
 scripts/qapi/schema.py         | 223 +++++++++++++++++----------------
 scripts/qapi/types.py          |  12 +-
 scripts/qapi/visit.py          |  24 ++--
 tests/qapi-schema/test-qapi.py |   9 +-
 9 files changed, 163 insertions(+), 147 deletions(-)

-- 
2.44.0



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/7] qapi: New QAPISchemaBranches, QAPISchemaAlternatives
  2024-03-20  7:43 [PATCH 0/7] qapi: Refactor QAPISchemaVariants Markus Armbruster
@ 2024-03-20  7:43 ` Markus Armbruster
  2024-03-20  7:43 ` [PATCH 2/7] qapi: Rename visitor parameter @variants to @branches Markus Armbruster
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2024-03-20  7:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: jsnow, peter.maydell, michael.roth

QAPISchemaVariants represents either a union type's branches, or an
alternate type's alternatives.  Much of its code is conditional on
which one it actually is.

Create QAPISchemaBranches for branches, and QAPISchemaAlternatives for
alternatives, both subtypes of QAPISchemaVariants.

Replace QAPISchemaVariants by one of them where possible.  Keep it
only where we actually deal with either of them.

QAPISchemaVariants.__init__() takes @tag_name and @tag_member, where
exactly one must be None: @tag_name for alternatives, @tag_member for
branches.  Let QAPISchemaBranches.__init__() take just @tag_name, and
QAPISchemaAlternatives.__init__() take just @tag_member.

A later patch will move the conditional code to the subtypes.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/introspect.py |  7 ++++---
 scripts/qapi/schema.py     | 32 ++++++++++++++++++++++++--------
 scripts/qapi/types.py      |  6 ++++--
 scripts/qapi/visit.py      | 11 ++++++-----
 4 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 4679b1bc2c..b866517942 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -26,6 +26,8 @@
 from .gen import QAPISchemaMonolithicCVisitor
 from .schema import (
     QAPISchema,
+    QAPISchemaAlternatives,
+    QAPISchemaBranches,
     QAPISchemaArrayType,
     QAPISchemaBuiltinType,
     QAPISchemaEntity,
@@ -36,7 +38,6 @@
     QAPISchemaObjectTypeMember,
     QAPISchemaType,
     QAPISchemaVariant,
-    QAPISchemaVariants,
 )
 from .source import QAPISourceInfo
 
@@ -335,7 +336,7 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
                                ifcond: QAPISchemaIfCond,
                                features: List[QAPISchemaFeature],
                                members: List[QAPISchemaObjectTypeMember],
-                               variants: Optional[QAPISchemaVariants]) -> None:
+                               variants: Optional[QAPISchemaBranches]) -> None:
         obj: SchemaInfoObject = {
             'members': [self._gen_object_member(m) for m in members]
         }
@@ -347,7 +348,7 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
     def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
                              ifcond: QAPISchemaIfCond,
                              features: List[QAPISchemaFeature],
-                             variants: QAPISchemaVariants) -> None:
+                             variants: QAPISchemaAlternatives) -> None:
         self._gen_tree(
             name, 'alternate',
             {'members': [Annotated({'type': self._use_type(m.type)},
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 5924947fc3..5cdedfc2c8 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -215,7 +215,7 @@ def visit_object_type(
         features: List[QAPISchemaFeature],
         base: Optional[QAPISchemaObjectType],
         members: List[QAPISchemaObjectTypeMember],
-        variants: Optional[QAPISchemaVariants],
+        variants: Optional[QAPISchemaBranches],
     ) -> None:
         pass
 
@@ -226,7 +226,7 @@ def visit_object_type_flat(
         ifcond: QAPISchemaIfCond,
         features: List[QAPISchemaFeature],
         members: List[QAPISchemaObjectTypeMember],
-        variants: Optional[QAPISchemaVariants],
+        variants: Optional[QAPISchemaBranches],
     ) -> None:
         pass
 
@@ -236,7 +236,7 @@ def visit_alternate_type(
         info: Optional[QAPISourceInfo],
         ifcond: QAPISchemaIfCond,
         features: List[QAPISchemaFeature],
-        variants: QAPISchemaVariants,
+        variants: QAPISchemaAlternatives,
     ) -> None:
         pass
 
@@ -524,7 +524,7 @@ def __init__(
         features: Optional[List[QAPISchemaFeature]],
         base: Optional[str],
         local_members: List[QAPISchemaObjectTypeMember],
-        variants: Optional[QAPISchemaVariants],
+        variants: Optional[QAPISchemaBranches],
     ):
         # struct has local_members, optional base, and no variants
         # union has base, variants, and no local_members
@@ -651,7 +651,7 @@ def __init__(
         doc: Optional[QAPIDoc],
         ifcond: Optional[QAPISchemaIfCond],
         features: List[QAPISchemaFeature],
-        variants: QAPISchemaVariants,
+        variants: QAPISchemaAlternatives,
     ):
         super().__init__(name, info, doc, ifcond, features)
         assert variants.tag_member
@@ -833,6 +833,22 @@ def check_clash(
             v.type.check_clash(info, dict(seen))
 
 
+class QAPISchemaBranches(QAPISchemaVariants):
+    def __init__(self,
+                 info: QAPISourceInfo,
+                 variants: List[QAPISchemaVariant],
+                 tag_name: str):
+        super().__init__(tag_name, info, None, variants)
+
+
+class QAPISchemaAlternatives(QAPISchemaVariants):
+    def __init__(self,
+                 info: QAPISourceInfo,
+                 variants: List[QAPISchemaVariant],
+                 tag_member: QAPISchemaObjectTypeMember):
+        super().__init__(None, info, tag_member, variants)
+
+
 class QAPISchemaMember:
     """ Represents object members, enum members and features """
     role = 'member'
@@ -1388,8 +1404,8 @@ def _def_union_type(self, expr: QAPIExpression) -> None:
         self._def_definition(
             QAPISchemaObjectType(name, info, expr.doc, ifcond, features,
                                  base, members,
-                                 QAPISchemaVariants(
-                                     tag_name, info, None, variants)))
+                                 QAPISchemaBranches(
+                                     info, variants, tag_name)))
 
     def _def_alternate_type(self, expr: QAPIExpression) -> None:
         name = expr['alternate']
@@ -1407,7 +1423,7 @@ def _def_alternate_type(self, expr: QAPIExpression) -> None:
         self._def_definition(
             QAPISchemaAlternateType(
                 name, info, expr.doc, ifcond, features,
-                QAPISchemaVariants(None, info, tag_member, variants)))
+                QAPISchemaAlternatives(info, variants, tag_member)))
 
     def _def_command(self, expr: QAPIExpression) -> None:
         name = expr['command']
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index c39d054d2c..23cdf3e83e 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -23,6 +23,8 @@
 )
 from .schema import (
     QAPISchema,
+    QAPISchemaAlternatives,
+    QAPISchemaBranches,
     QAPISchemaEnumMember,
     QAPISchemaFeature,
     QAPISchemaIfCond,
@@ -348,7 +350,7 @@ def visit_object_type(self,
                           features: List[QAPISchemaFeature],
                           base: Optional[QAPISchemaObjectType],
                           members: List[QAPISchemaObjectTypeMember],
-                          variants: Optional[QAPISchemaVariants]) -> None:
+                          variants: Optional[QAPISchemaBranches]) -> None:
         # Nothing to do for the special empty builtin
         if name == 'q_empty':
             return
@@ -369,7 +371,7 @@ def visit_alternate_type(self,
                              info: Optional[QAPISourceInfo],
                              ifcond: QAPISchemaIfCond,
                              features: List[QAPISchemaFeature],
-                             variants: QAPISchemaVariants) -> None:
+                             variants: QAPISchemaAlternatives) -> None:
         with ifcontext(ifcond, self._genh):
             self._genh.preamble_add(gen_fwd_object_or_array(name))
         self._genh.add(gen_object(name, ifcond, None,
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index c56ea4d724..746735e013 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -28,6 +28,8 @@
 )
 from .schema import (
     QAPISchema,
+    QAPISchemaAlternatives,
+    QAPISchemaBranches,
     QAPISchemaEnumMember,
     QAPISchemaEnumType,
     QAPISchemaFeature,
@@ -35,7 +37,6 @@
     QAPISchemaObjectType,
     QAPISchemaObjectTypeMember,
     QAPISchemaType,
-    QAPISchemaVariants,
 )
 from .source import QAPISourceInfo
 
@@ -63,7 +64,7 @@ def gen_visit_members_decl(name: str) -> str:
 def gen_visit_object_members(name: str,
                              base: Optional[QAPISchemaObjectType],
                              members: List[QAPISchemaObjectTypeMember],
-                             variants: Optional[QAPISchemaVariants]) -> str:
+                             variants: Optional[QAPISchemaBranches]) -> str:
     ret = mcgen('''
 
 bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
@@ -222,7 +223,7 @@ def gen_visit_enum(name: str) -> str:
                  c_name=c_name(name))
 
 
-def gen_visit_alternate(name: str, variants: QAPISchemaVariants) -> str:
+def gen_visit_alternate(name: str, variants: QAPISchemaAlternatives) -> str:
     ret = mcgen('''
 
 bool visit_type_%(c_name)s(Visitor *v, const char *name,
@@ -394,7 +395,7 @@ def visit_object_type(self,
                           features: List[QAPISchemaFeature],
                           base: Optional[QAPISchemaObjectType],
                           members: List[QAPISchemaObjectTypeMember],
-                          variants: Optional[QAPISchemaVariants]) -> None:
+                          variants: Optional[QAPISchemaBranches]) -> None:
         # Nothing to do for the special empty builtin
         if name == 'q_empty':
             return
@@ -414,7 +415,7 @@ def visit_alternate_type(self,
                              info: Optional[QAPISourceInfo],
                              ifcond: QAPISchemaIfCond,
                              features: List[QAPISchemaFeature],
-                             variants: QAPISchemaVariants) -> None:
+                             variants: QAPISchemaAlternatives) -> None:
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.add(gen_visit_decl(name))
             self._genc.add(gen_visit_alternate(name, variants))
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/7] qapi: Rename visitor parameter @variants to @branches
  2024-03-20  7:43 [PATCH 0/7] qapi: Refactor QAPISchemaVariants Markus Armbruster
  2024-03-20  7:43 ` [PATCH 1/7] qapi: New QAPISchemaBranches, QAPISchemaAlternatives Markus Armbruster
@ 2024-03-20  7:43 ` Markus Armbruster
  2024-03-20  7:43 ` [PATCH 3/7] qapi: Rename visitor parameter @variants to @alternatives Markus Armbruster
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2024-03-20  7:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: jsnow, peter.maydell, michael.roth

The previous commit narrowed the type of .visit_object_type()
parameter @variants from QAPISchemaVariants to QAPISchemaBranches.
Rename it to @branches.

Same for .visit_object_type_flat().

A few of these pass @branches to helper functions:
QAPISchemaGenRSTVisitor.visit_object_type() to ._nodes_for_members()
and ._nodes_for_variant_when(), and
QAPISchemaGenVisitVisitor.visit_object_type() to
gen_visit_object_members().  Rename the helpers' @variants parameters
to @branches as well.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/sphinx/qapidoc.py         | 18 +++++++++---------
 scripts/qapi/introspect.py     |  8 ++++----
 scripts/qapi/schema.py         |  4 ++--
 scripts/qapi/types.py          |  4 ++--
 scripts/qapi/visit.py          | 12 ++++++------
 tests/qapi-schema/test-qapi.py |  4 ++--
 6 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 8d428c64b0..71362ba929 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -145,22 +145,22 @@ def _nodes_for_one_member(self, member):
             term.extend(self._nodes_for_ifcond(member.ifcond))
         return term
 
-    def _nodes_for_variant_when(self, variants, variant):
+    def _nodes_for_variant_when(self, branches, variant):
         """Return list of Text, literal nodes for variant 'when' clause
 
         Return a list of doctree nodes which give text like
         'when tagname is variant (If: ...)' suitable for use in
-        the 'variants' part of a definition list.
+        the 'branches' part of a definition list.
         """
         term = [nodes.Text(' when '),
-                nodes.literal('', variants.tag_member.name),
+                nodes.literal('', branches.tag_member.name),
                 nodes.Text(' is '),
                 nodes.literal('', '"%s"' % variant.name)]
         if variant.ifcond.is_present():
             term.extend(self._nodes_for_ifcond(variant.ifcond))
         return term
 
-    def _nodes_for_members(self, doc, what, base=None, variants=None):
+    def _nodes_for_members(self, doc, what, base=None, branches=None):
         """Return list of doctree nodes for the table of members"""
         dlnode = nodes.definition_list()
         for section in doc.args.values():
@@ -178,14 +178,14 @@ def _nodes_for_members(self, doc, what, base=None, variants=None):
                                          nodes.literal('', base.doc_type())],
                                         None)
 
-        if variants:
-            for v in variants.variants:
+        if branches:
+            for v in branches.variants:
                 if v.type.name == 'q_empty':
                     continue
                 assert not v.type.is_implicit()
                 term = [nodes.Text('The members of '),
                         nodes.literal('', v.type.doc_type())]
-                term.extend(self._nodes_for_variant_when(variants, v))
+                term.extend(self._nodes_for_variant_when(branches, v))
                 dlnode += self._make_dlitem(term, None)
 
         if not dlnode.children:
@@ -308,12 +308,12 @@ def visit_enum_type(self, name, info, ifcond, features, members, prefix):
                       + self._nodes_for_if_section(ifcond))
 
     def visit_object_type(self, name, info, ifcond, features,
-                          base, members, variants):
+                          base, members, branches):
         doc = self._cur_doc
         if base and base.is_implicit():
             base = None
         self._add_doc('Object',
-                      self._nodes_for_members(doc, 'Members', base, variants)
+                      self._nodes_for_members(doc, 'Members', base, branches)
                       + self._nodes_for_features(doc)
                       + self._nodes_for_sections(doc)
                       + self._nodes_for_if_section(ifcond))
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index b866517942..7852591490 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -336,13 +336,13 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
                                ifcond: QAPISchemaIfCond,
                                features: List[QAPISchemaFeature],
                                members: List[QAPISchemaObjectTypeMember],
-                               variants: Optional[QAPISchemaBranches]) -> None:
+                               branches: Optional[QAPISchemaBranches]) -> None:
         obj: SchemaInfoObject = {
             'members': [self._gen_object_member(m) for m in members]
         }
-        if variants:
-            obj['tag'] = variants.tag_member.name
-            obj['variants'] = [self._gen_variant(v) for v in variants.variants]
+        if branches:
+            obj['tag'] = branches.tag_member.name
+            obj['variants'] = [self._gen_variant(v) for v in branches.variants]
         self._gen_tree(name, 'object', obj, ifcond, features)
 
     def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 5cdedfc2c8..65c82dd4f1 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -215,7 +215,7 @@ def visit_object_type(
         features: List[QAPISchemaFeature],
         base: Optional[QAPISchemaObjectType],
         members: List[QAPISchemaObjectTypeMember],
-        variants: Optional[QAPISchemaBranches],
+        branches: Optional[QAPISchemaBranches],
     ) -> None:
         pass
 
@@ -226,7 +226,7 @@ def visit_object_type_flat(
         ifcond: QAPISchemaIfCond,
         features: List[QAPISchemaFeature],
         members: List[QAPISchemaObjectTypeMember],
-        variants: Optional[QAPISchemaBranches],
+        branches: Optional[QAPISchemaBranches],
     ) -> None:
         pass
 
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 23cdf3e83e..0abb78f3a8 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -350,13 +350,13 @@ def visit_object_type(self,
                           features: List[QAPISchemaFeature],
                           base: Optional[QAPISchemaObjectType],
                           members: List[QAPISchemaObjectTypeMember],
-                          variants: Optional[QAPISchemaBranches]) -> None:
+                          branches: Optional[QAPISchemaBranches]) -> None:
         # Nothing to do for the special empty builtin
         if name == 'q_empty':
             return
         with ifcontext(ifcond, self._genh):
             self._genh.preamble_add(gen_fwd_object_or_array(name))
-        self._genh.add(gen_object(name, ifcond, base, members, variants))
+        self._genh.add(gen_object(name, ifcond, base, members, branches))
         with ifcontext(ifcond, self._genh, self._genc):
             if base and not base.is_implicit():
                 self._genh.add(gen_upcast(name, base))
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 746735e013..cbaec4cf90 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -64,7 +64,7 @@ def gen_visit_members_decl(name: str) -> str:
 def gen_visit_object_members(name: str,
                              base: Optional[QAPISchemaObjectType],
                              members: List[QAPISchemaObjectTypeMember],
-                             variants: Optional[QAPISchemaBranches]) -> str:
+                             branches: Optional[QAPISchemaBranches]) -> str:
     ret = mcgen('''
 
 bool visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp)
@@ -132,8 +132,8 @@ def gen_visit_object_members(name: str,
 ''')
         ret += memb.ifcond.gen_endif()
 
-    if variants:
-        tag_member = variants.tag_member
+    if branches:
+        tag_member = branches.tag_member
         assert isinstance(tag_member.type, QAPISchemaEnumType)
 
         ret += mcgen('''
@@ -141,7 +141,7 @@ def gen_visit_object_members(name: str,
 ''',
                      c_name=c_name(tag_member.name))
 
-        for var in variants.variants:
+        for var in branches.variants:
             case_str = c_enum_const(tag_member.type.name, var.name,
                                     tag_member.type.prefix)
             ret += var.ifcond.gen_if()
@@ -395,14 +395,14 @@ def visit_object_type(self,
                           features: List[QAPISchemaFeature],
                           base: Optional[QAPISchemaObjectType],
                           members: List[QAPISchemaObjectTypeMember],
-                          variants: Optional[QAPISchemaBranches]) -> None:
+                          branches: Optional[QAPISchemaBranches]) -> None:
         # Nothing to do for the special empty builtin
         if name == 'q_empty':
             return
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.add(gen_visit_members_decl(name))
             self._genc.add(gen_visit_object_members(name, base,
-                                                    members, variants))
+                                                    members, branches))
             # TODO Worth changing the visitor signature, so we could
             # directly use rather than repeat type.is_implicit()?
             if not name.startswith('q_'):
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 40095431ae..7c67ad8d9b 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -48,7 +48,7 @@ def visit_array_type(self, name, info, ifcond, element_type):
         self._print_if(ifcond)
 
     def visit_object_type(self, name, info, ifcond, features,
-                          base, members, variants):
+                          base, members, branches):
         print('object %s' % name)
         if base:
             print('    base %s' % base.name)
@@ -57,7 +57,7 @@ def visit_object_type(self, name, info, ifcond, features,
                   % (m.name, m.type.name, m.optional))
             self._print_if(m.ifcond, 8)
             self._print_features(m.features, indent=8)
-        self._print_variants(variants)
+        self._print_variants(branches)
         self._print_if(ifcond)
         self._print_features(features)
 
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/7] qapi: Rename visitor parameter @variants to @alternatives
  2024-03-20  7:43 [PATCH 0/7] qapi: Refactor QAPISchemaVariants Markus Armbruster
  2024-03-20  7:43 ` [PATCH 1/7] qapi: New QAPISchemaBranches, QAPISchemaAlternatives Markus Armbruster
  2024-03-20  7:43 ` [PATCH 2/7] qapi: Rename visitor parameter @variants to @branches Markus Armbruster
@ 2024-03-20  7:43 ` Markus Armbruster
  2024-03-20  7:43 ` [PATCH 4/7] qapi: Rename QAPISchemaObjectType.variants to .branches Markus Armbruster
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2024-03-20  7:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: jsnow, peter.maydell, michael.roth

A previous commit narrowed the type of .visit_alternate_type()
parameter @variants from QAPISchemaVariants to QAPISchemaAlternatives.
Rename it to @alternatives.

One of them passes @alternatives to helper function
gen_visit_alternate().  Rename its @variants parameter to
@alternatives as well.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/sphinx/qapidoc.py         | 3 ++-
 scripts/qapi/introspect.py     | 4 ++--
 scripts/qapi/schema.py         | 2 +-
 scripts/qapi/types.py          | 4 ++--
 scripts/qapi/visit.py          | 9 +++++----
 tests/qapi-schema/test-qapi.py | 5 +++--
 6 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 71362ba929..f270b494f0 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -318,7 +318,8 @@ def visit_object_type(self, name, info, ifcond, features,
                       + self._nodes_for_sections(doc)
                       + self._nodes_for_if_section(ifcond))
 
-    def visit_alternate_type(self, name, info, ifcond, features, variants):
+    def visit_alternate_type(self, name, info, ifcond, features,
+                             alternatives):
         doc = self._cur_doc
         self._add_doc('Alternate',
                       self._nodes_for_members(doc, 'Members')
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 7852591490..86c075a6ad 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -348,12 +348,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
     def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
                              ifcond: QAPISchemaIfCond,
                              features: List[QAPISchemaFeature],
-                             variants: QAPISchemaAlternatives) -> None:
+                             alternatives: QAPISchemaAlternatives) -> None:
         self._gen_tree(
             name, 'alternate',
             {'members': [Annotated({'type': self._use_type(m.type)},
                                    m.ifcond)
-                         for m in variants.variants]},
+                         for m in alternatives.variants]},
             ifcond, features
         )
 
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 65c82dd4f1..2b67992aee 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -236,7 +236,7 @@ def visit_alternate_type(
         info: Optional[QAPISourceInfo],
         ifcond: QAPISchemaIfCond,
         features: List[QAPISchemaFeature],
-        variants: QAPISchemaAlternatives,
+        alternatives: QAPISchemaAlternatives,
     ) -> None:
         pass
 
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 0abb78f3a8..69f5f6ffd0 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -371,11 +371,11 @@ def visit_alternate_type(self,
                              info: Optional[QAPISourceInfo],
                              ifcond: QAPISchemaIfCond,
                              features: List[QAPISchemaFeature],
-                             variants: QAPISchemaAlternatives) -> None:
+                             alternatives: QAPISchemaAlternatives) -> None:
         with ifcontext(ifcond, self._genh):
             self._genh.preamble_add(gen_fwd_object_or_array(name))
         self._genh.add(gen_object(name, ifcond, None,
-                                  [variants.tag_member], variants))
+                                  [alternatives.tag_member], alternatives))
         with ifcontext(ifcond, self._genh, self._genc):
             self._gen_type_cleanup(name)
 
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index cbaec4cf90..4eae5628e6 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -223,7 +223,8 @@ def gen_visit_enum(name: str) -> str:
                  c_name=c_name(name))
 
 
-def gen_visit_alternate(name: str, variants: QAPISchemaAlternatives) -> str:
+def gen_visit_alternate(name: str,
+                        alternatives: QAPISchemaAlternatives) -> str:
     ret = mcgen('''
 
 bool visit_type_%(c_name)s(Visitor *v, const char *name,
@@ -245,7 +246,7 @@ def gen_visit_alternate(name: str, variants: QAPISchemaAlternatives) -> str:
 ''',
                 c_name=c_name(name))
 
-    for var in variants.variants:
+    for var in alternatives.variants:
         ret += var.ifcond.gen_if()
         ret += mcgen('''
     case %(case)s:
@@ -415,10 +416,10 @@ def visit_alternate_type(self,
                              info: Optional[QAPISourceInfo],
                              ifcond: QAPISchemaIfCond,
                              features: List[QAPISchemaFeature],
-                             variants: QAPISchemaAlternatives) -> None:
+                             alternatives: QAPISchemaAlternatives) -> None:
         with ifcontext(ifcond, self._genh, self._genc):
             self._genh.add(gen_visit_decl(name))
-            self._genc.add(gen_visit_alternate(name, variants))
+            self._genc.add(gen_visit_alternate(name, alternatives))
 
 
 def gen_visit(schema: QAPISchema,
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index 7c67ad8d9b..7e3f9f4aa1 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -61,9 +61,10 @@ def visit_object_type(self, name, info, ifcond, features,
         self._print_if(ifcond)
         self._print_features(features)
 
-    def visit_alternate_type(self, name, info, ifcond, features, variants):
+    def visit_alternate_type(self, name, info, ifcond, features,
+                             alternatives):
         print('alternate %s' % name)
-        self._print_variants(variants)
+        self._print_variants(alternatives)
         self._print_if(ifcond)
         self._print_features(features)
 
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 4/7] qapi: Rename QAPISchemaObjectType.variants to .branches
  2024-03-20  7:43 [PATCH 0/7] qapi: Refactor QAPISchemaVariants Markus Armbruster
                   ` (2 preceding siblings ...)
  2024-03-20  7:43 ` [PATCH 3/7] qapi: Rename visitor parameter @variants to @alternatives Markus Armbruster
@ 2024-03-20  7:43 ` Markus Armbruster
  2024-03-20  7:43 ` [PATCH 5/7] qapi: Rename QAPISchemaAlternateType.variants to .alternatives Markus Armbruster
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2024-03-20  7:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: jsnow, peter.maydell, michael.roth

A previous commit narrowed the type of QAPISchemaObjectType.variants
from QAPISchemaVariants to QAPISchemaBranches.  Rename it to
.branches.

Same for .__init__() parameter @variants.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/commands.py |  2 +-
 scripts/qapi/events.py   |  2 +-
 scripts/qapi/gen.py      |  2 +-
 scripts/qapi/schema.py   | 36 ++++++++++++++++++------------------
 scripts/qapi/types.py    |  2 +-
 5 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index d1fdf4182c..79951a841f 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -64,7 +64,7 @@ def gen_call(name: str,
         assert arg_type
         argstr = '&arg, '
     elif arg_type:
-        assert not arg_type.variants
+        assert not arg_type.branches
         for memb in arg_type.members:
             assert not memb.ifcond.is_present()
             if memb.need_has():
diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
index 3cf01e96b6..d1f639981a 100644
--- a/scripts/qapi/events.py
+++ b/scripts/qapi/events.py
@@ -51,7 +51,7 @@ def gen_param_var(typ: QAPISchemaObjectType) -> str:
 
     Initialize it with the function arguments defined in `gen_event_send`.
     """
-    assert not typ.variants
+    assert not typ.branches
     ret = mcgen('''
     %(c_name)s param = {
 ''',
diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index 5412716617..6a8abe0041 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -118,7 +118,7 @@ def build_params(arg_type: Optional[QAPISchemaObjectType],
         ret += '%s arg' % arg_type.c_param_type()
         sep = ', '
     elif arg_type:
-        assert not arg_type.variants
+        assert not arg_type.branches
         for memb in arg_type.members:
             assert not memb.ifcond.is_present()
             ret += sep
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 2b67992aee..c9ff794d0c 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -524,20 +524,20 @@ def __init__(
         features: Optional[List[QAPISchemaFeature]],
         base: Optional[str],
         local_members: List[QAPISchemaObjectTypeMember],
-        variants: Optional[QAPISchemaBranches],
+        branches: Optional[QAPISchemaBranches],
     ):
-        # struct has local_members, optional base, and no variants
-        # union has base, variants, and no local_members
+        # struct has local_members, optional base, and no branches
+        # union has base, branches, and no local_members
         super().__init__(name, info, doc, ifcond, features)
-        self.meta = 'union' if variants else 'struct'
+        self.meta = 'union' if branches else 'struct'
         for m in local_members:
             m.set_defined_in(name)
-        if variants is not None:
-            variants.set_defined_in(name)
+        if branches is not None:
+            branches.set_defined_in(name)
         self._base_name = base
         self.base = None
         self.local_members = local_members
-        self.variants = variants
+        self.branches = branches
         self.members: List[QAPISchemaObjectTypeMember]
         self._check_complete = False
 
@@ -561,7 +561,7 @@ def check(self, schema: QAPISchema) -> None:
             self.base = schema.resolve_type(self._base_name, self.info,
                                             "'base'")
             if (not isinstance(self.base, QAPISchemaObjectType)
-                    or self.base.variants):
+                    or self.base.branches):
                 raise QAPISemError(
                     self.info,
                     "'base' requires a struct type, %s isn't"
@@ -577,9 +577,9 @@ def check(self, schema: QAPISchema) -> None:
         # Cast down to the subtype.
         members = cast(List[QAPISchemaObjectTypeMember], list(seen.values()))
 
-        if self.variants:
-            self.variants.check(schema, seen)
-            self.variants.check_clash(self.info, seen)
+        if self.branches:
+            self.branches.check(schema, seen)
+            self.branches.check_clash(self.info, seen)
 
         self.members = members
         self._check_complete = True  # mark completed
@@ -595,8 +595,8 @@ def check_clash(
         assert self._checked
         for m in self.members:
             m.check_clash(info, seen)
-        if self.variants:
-            self.variants.check_clash(info, seen)
+        if self.branches:
+            self.branches.check_clash(info, seen)
 
     def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
@@ -612,7 +612,7 @@ def is_implicit(self) -> bool:
         return self.name.startswith('q_')
 
     def is_empty(self) -> bool:
-        return not self.members and not self.variants
+        return not self.members and not self.branches
 
     def has_conditional_members(self) -> bool:
         return any(m.ifcond.is_present() for m in self.members)
@@ -635,10 +635,10 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
         super().visit(visitor)
         visitor.visit_object_type(
             self.name, self.info, self.ifcond, self.features,
-            self.base, self.local_members, self.variants)
+            self.base, self.local_members, self.branches)
         visitor.visit_object_type_flat(
             self.name, self.info, self.ifcond, self.features,
-            self.members, self.variants)
+            self.members, self.branches)
 
 
 class QAPISchemaAlternateType(QAPISchemaType):
@@ -1035,7 +1035,7 @@ def check(self, schema: QAPISchema) -> None:
                     "command's 'data' cannot take %s"
                     % arg_type.describe())
             self.arg_type = arg_type
-            if self.arg_type.variants and not self.boxed:
+            if self.arg_type.branches and not self.boxed:
                 raise QAPISemError(
                     self.info,
                     "command's 'data' can take %s only with 'boxed': true"
@@ -1103,7 +1103,7 @@ def check(self, schema: QAPISchema) -> None:
                     "event's 'data' cannot take %s"
                     % typ.describe())
             self.arg_type = typ
-            if self.arg_type.variants and not self.boxed:
+            if self.arg_type.branches and not self.boxed:
                 raise QAPISemError(
                     self.info,
                     "event's 'data' can take %s only with 'boxed': true"
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 69f5f6ffd0..0dd0b00ada 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -171,7 +171,7 @@ def gen_object(name: str, ifcond: QAPISchemaIfCond,
         if not isinstance(obj, QAPISchemaObjectType):
             continue
         ret += gen_object(obj.name, obj.ifcond, obj.base,
-                          obj.local_members, obj.variants)
+                          obj.local_members, obj.branches)
 
     ret += mcgen('''
 
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 5/7] qapi: Rename QAPISchemaAlternateType.variants to .alternatives
  2024-03-20  7:43 [PATCH 0/7] qapi: Refactor QAPISchemaVariants Markus Armbruster
                   ` (3 preceding siblings ...)
  2024-03-20  7:43 ` [PATCH 4/7] qapi: Rename QAPISchemaObjectType.variants to .branches Markus Armbruster
@ 2024-03-20  7:43 ` Markus Armbruster
  2024-03-20  7:43 ` [PATCH 6/7] qapi: Move conditional code from QAPISchemaVariants to its subtypes Markus Armbruster
  2024-03-20  7:43 ` [PATCH 7/7] qapi: Simplify QAPISchemaVariants @tag_member Markus Armbruster
  6 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2024-03-20  7:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: jsnow, peter.maydell, michael.roth

A previous commit narrowed the type of
QAPISchemaAlternateType.variants from QAPISchemaVariants to
QAPISchemaAlternatives.  Rename it to .alternatives.

Same for .__init__() parameter @variants.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/schema.py | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index c9ff794d0c..9bdbfd52b2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -651,25 +651,25 @@ def __init__(
         doc: Optional[QAPIDoc],
         ifcond: Optional[QAPISchemaIfCond],
         features: List[QAPISchemaFeature],
-        variants: QAPISchemaAlternatives,
+        alternatives: QAPISchemaAlternatives,
     ):
         super().__init__(name, info, doc, ifcond, features)
-        assert variants.tag_member
-        variants.set_defined_in(name)
-        variants.tag_member.set_defined_in(self.name)
-        self.variants = variants
+        assert alternatives.tag_member
+        alternatives.set_defined_in(name)
+        alternatives.tag_member.set_defined_in(self.name)
+        self.alternatives = alternatives
 
     def check(self, schema: QAPISchema) -> None:
         super().check(schema)
-        self.variants.tag_member.check(schema)
-        # Not calling self.variants.check_clash(), because there's nothing
-        # to clash with
-        self.variants.check(schema, {})
+        self.alternatives.tag_member.check(schema)
+        # Not calling self.alternatives.check_clash(), because there's
+        # nothing to clash with
+        self.alternatives.check(schema, {})
         # Alternate branch names have no relation to the tag enum values;
         # so we have to check for potential name collisions ourselves.
         seen: Dict[str, QAPISchemaMember] = {}
         types_seen: Dict[str, str] = {}
-        for v in self.variants.variants:
+        for v in self.alternatives.variants:
             v.check_clash(self.info, seen)
             qtype = v.type.alternate_qtype()
             if not qtype:
@@ -700,7 +700,7 @@ def check(self, schema: QAPISchema) -> None:
     def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
-        for v in self.variants.variants:
+        for v in self.alternatives.variants:
             v.connect_doc(doc)
 
     def c_type(self) -> str:
@@ -712,7 +712,8 @@ def json_type(self) -> str:
     def visit(self, visitor: QAPISchemaVisitor) -> None:
         super().visit(visitor)
         visitor.visit_alternate_type(
-            self.name, self.info, self.ifcond, self.features, self.variants)
+            self.name, self.info, self.ifcond, self.features,
+            self.alternatives)
 
 
 class QAPISchemaVariants:
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 6/7] qapi: Move conditional code from QAPISchemaVariants to its subtypes
  2024-03-20  7:43 [PATCH 0/7] qapi: Refactor QAPISchemaVariants Markus Armbruster
                   ` (4 preceding siblings ...)
  2024-03-20  7:43 ` [PATCH 5/7] qapi: Rename QAPISchemaAlternateType.variants to .alternatives Markus Armbruster
@ 2024-03-20  7:43 ` Markus Armbruster
  2024-03-20  7:43 ` [PATCH 7/7] qapi: Simplify QAPISchemaVariants @tag_member Markus Armbruster
  6 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2024-03-20  7:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: jsnow, peter.maydell, michael.roth

QAPISchemaVariants.check()'s code is almost entirely conditional on
union vs. alternate type.

Move the conditional code to QAPISchemaBranches.check() and
QAPISchemaAlternatives.check(), where the conditions are always
satisfied.

Attribute QAPISchemaVariants.tag_name is now only used by
QAPISchemaBranches.  Move it there.

Refactor the three types' .__init__() to make them a bit simpler.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/schema.py | 138 ++++++++++++++++++++---------------------
 1 file changed, 69 insertions(+), 69 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 9bdbfd52b2..c5b824f1fd 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -719,20 +719,11 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
 class QAPISchemaVariants:
     def __init__(
         self,
-        tag_name: Optional[str],
         info: QAPISourceInfo,
-        tag_member: Optional[QAPISchemaObjectTypeMember],
         variants: List[QAPISchemaVariant],
     ):
-        # Unions pass tag_name but not tag_member.
-        # Alternates pass tag_member but not tag_name.
-        # After check(), tag_member is always set.
-        assert bool(tag_member) != bool(tag_name)
-        assert (isinstance(tag_name, str) or
-                isinstance(tag_member, QAPISchemaObjectTypeMember))
-        self._tag_name = tag_name
         self.info = info
-        self._tag_member = tag_member
+        self._tag_member: Optional[QAPISchemaObjectTypeMember] = None
         self.variants = variants
 
     @property
@@ -749,58 +740,66 @@ def set_defined_in(self, name: str) -> None:
             v.set_defined_in(name)
 
     def check(
-        self, schema: QAPISchema, seen: Dict[str, QAPISchemaMember]
+            self, schema: QAPISchema, seen: Dict[str, QAPISchemaMember]
     ) -> None:
-        if self._tag_name:      # union
-            # We need to narrow the member type:
-            tmp = seen.get(c_name(self._tag_name))
-            assert tmp is None or isinstance(tmp, QAPISchemaObjectTypeMember)
-            self._tag_member = tmp
+        for v in self.variants:
+            v.check(schema)
 
-            base = "'base'"
-            # Pointing to the base type when not implicit would be
-            # nice, but we don't know it here
-            if not self._tag_member or self._tag_name != self._tag_member.name:
-                raise QAPISemError(
-                    self.info,
-                    "discriminator '%s' is not a member of %s"
-                    % (self._tag_name, base))
-            # Here we do:
-            assert self.tag_member.defined_in
-            base_type = schema.lookup_type(self.tag_member.defined_in)
-            assert base_type
-            if not base_type.is_implicit():
-                base = "base type '%s'" % self.tag_member.defined_in
-            if not isinstance(self.tag_member.type, QAPISchemaEnumType):
-                raise QAPISemError(
-                    self.info,
-                    "discriminator member '%s' of %s must be of enum type"
-                    % (self._tag_name, base))
-            if self.tag_member.optional:
-                raise QAPISemError(
-                    self.info,
-                    "discriminator member '%s' of %s must not be optional"
-                    % (self._tag_name, base))
-            if self.tag_member.ifcond.is_present():
-                raise QAPISemError(
-                    self.info,
-                    "discriminator member '%s' of %s must not be conditional"
-                    % (self._tag_name, base))
-        else:                   # alternate
-            assert self._tag_member
-            assert isinstance(self.tag_member.type, QAPISchemaEnumType)
-            assert not self.tag_member.optional
-            assert not self.tag_member.ifcond.is_present()
-        if self._tag_name:      # union
-            # branches that are not explicitly covered get an empty type
-            assert self.tag_member.defined_in
-            cases = {v.name for v in self.variants}
-            for m in self.tag_member.type.members:
-                if m.name not in cases:
-                    v = QAPISchemaVariant(m.name, self.info,
-                                          'q_empty', m.ifcond)
-                    v.set_defined_in(self.tag_member.defined_in)
-                    self.variants.append(v)
+
+class QAPISchemaBranches(QAPISchemaVariants):
+    def __init__(self,
+                 info: QAPISourceInfo,
+                 variants: List[QAPISchemaVariant],
+                 tag_name: str):
+        super().__init__(info, variants)
+        self._tag_name = tag_name
+
+    def check(
+            self, schema: QAPISchema, seen: Dict[str, QAPISchemaMember]
+    ) -> None:
+        # We need to narrow the member type:
+        tmp = seen.get(c_name(self._tag_name))
+        assert tmp is None or isinstance(tmp, QAPISchemaObjectTypeMember)
+        self._tag_member = tmp
+
+        base = "'base'"
+        # Pointing to the base type when not implicit would be
+        # nice, but we don't know it here
+        if not self._tag_member or self._tag_name != self._tag_member.name:
+            raise QAPISemError(
+                self.info,
+                "discriminator '%s' is not a member of %s"
+                % (self._tag_name, base))
+        # Here we do:
+        assert self.tag_member.defined_in
+        base_type = schema.lookup_type(self.tag_member.defined_in)
+        assert base_type
+        if not base_type.is_implicit():
+            base = "base type '%s'" % self.tag_member.defined_in
+        if not isinstance(self.tag_member.type, QAPISchemaEnumType):
+            raise QAPISemError(
+                self.info,
+                "discriminator member '%s' of %s must be of enum type"
+                % (self._tag_name, base))
+        if self.tag_member.optional:
+            raise QAPISemError(
+                self.info,
+                "discriminator member '%s' of %s must not be optional"
+                % (self._tag_name, base))
+        if self.tag_member.ifcond.is_present():
+            raise QAPISemError(
+                self.info,
+                "discriminator member '%s' of %s must not be conditional"
+                % (self._tag_name, base))
+        # branches that are not explicitly covered get an empty type
+        assert self.tag_member.defined_in
+        cases = {v.name for v in self.variants}
+        for m in self.tag_member.type.members:
+            if m.name not in cases:
+                v = QAPISchemaVariant(m.name, self.info,
+                                      'q_empty', m.ifcond)
+                v.set_defined_in(self.tag_member.defined_in)
+                self.variants.append(v)
         if not self.variants:
             raise QAPISemError(self.info, "union has no branches")
         for v in self.variants:
@@ -834,20 +833,21 @@ def check_clash(
             v.type.check_clash(info, dict(seen))
 
 
-class QAPISchemaBranches(QAPISchemaVariants):
-    def __init__(self,
-                 info: QAPISourceInfo,
-                 variants: List[QAPISchemaVariant],
-                 tag_name: str):
-        super().__init__(tag_name, info, None, variants)
-
-
 class QAPISchemaAlternatives(QAPISchemaVariants):
     def __init__(self,
                  info: QAPISourceInfo,
                  variants: List[QAPISchemaVariant],
                  tag_member: QAPISchemaObjectTypeMember):
-        super().__init__(None, info, tag_member, variants)
+        super().__init__(info, variants)
+        self._tag_member = tag_member
+
+    def check(
+            self, schema: QAPISchema, seen: Dict[str, QAPISchemaMember]
+    ) -> None:
+        super().check(schema, seen)
+        assert isinstance(self.tag_member.type, QAPISchemaEnumType)
+        assert not self.tag_member.optional
+        assert not self.tag_member.ifcond.is_present()
 
 
 class QAPISchemaMember:
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 7/7] qapi: Simplify QAPISchemaVariants @tag_member
  2024-03-20  7:43 [PATCH 0/7] qapi: Refactor QAPISchemaVariants Markus Armbruster
                   ` (5 preceding siblings ...)
  2024-03-20  7:43 ` [PATCH 6/7] qapi: Move conditional code from QAPISchemaVariants to its subtypes Markus Armbruster
@ 2024-03-20  7:43 ` Markus Armbruster
  6 siblings, 0 replies; 8+ messages in thread
From: Markus Armbruster @ 2024-03-20  7:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: jsnow, peter.maydell, michael.roth

For union types, the tag member is known only after .check().

We used to code this in a simple way: QAPISchemaVariants attribute
.tag_member was None for union types until .check().

Since this complicated typing, recent commit "qapi/schema: fix typing
for QAPISchemaVariants.tag_member" hid it behind a property.

The previous commit lets us treat .tag_member just like the other
attributes that become known only in .check(): declare, but don't
initialize it in .__init__().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/schema.py | 44 +++++++++++++++++-------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index c5b824f1fd..721c470d2b 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -723,18 +723,9 @@ def __init__(
         variants: List[QAPISchemaVariant],
     ):
         self.info = info
-        self._tag_member: Optional[QAPISchemaObjectTypeMember] = None
+        self.tag_member: QAPISchemaObjectTypeMember
         self.variants = variants
 
-    @property
-    def tag_member(self) -> QAPISchemaObjectTypeMember:
-        if self._tag_member is None:
-            raise RuntimeError(
-                "QAPISchemaVariants has no tag_member property until "
-                "after check() has been run."
-            )
-        return self._tag_member
-
     def set_defined_in(self, name: str) -> None:
         for v in self.variants:
             v.set_defined_in(name)
@@ -758,47 +749,48 @@ def check(
             self, schema: QAPISchema, seen: Dict[str, QAPISchemaMember]
     ) -> None:
         # We need to narrow the member type:
-        tmp = seen.get(c_name(self._tag_name))
-        assert tmp is None or isinstance(tmp, QAPISchemaObjectTypeMember)
-        self._tag_member = tmp
+        tag_member = seen.get(c_name(self._tag_name))
+        assert (tag_member is None
+                or isinstance(tag_member, QAPISchemaObjectTypeMember))
 
         base = "'base'"
         # Pointing to the base type when not implicit would be
         # nice, but we don't know it here
-        if not self._tag_member or self._tag_name != self._tag_member.name:
+        if not tag_member or self._tag_name != tag_member.name:
             raise QAPISemError(
                 self.info,
                 "discriminator '%s' is not a member of %s"
                 % (self._tag_name, base))
+        self.tag_member = tag_member
         # Here we do:
-        assert self.tag_member.defined_in
-        base_type = schema.lookup_type(self.tag_member.defined_in)
+        assert tag_member.defined_in
+        base_type = schema.lookup_type(tag_member.defined_in)
         assert base_type
         if not base_type.is_implicit():
-            base = "base type '%s'" % self.tag_member.defined_in
-        if not isinstance(self.tag_member.type, QAPISchemaEnumType):
+            base = "base type '%s'" % tag_member.defined_in
+        if not isinstance(tag_member.type, QAPISchemaEnumType):
             raise QAPISemError(
                 self.info,
                 "discriminator member '%s' of %s must be of enum type"
                 % (self._tag_name, base))
-        if self.tag_member.optional:
+        if tag_member.optional:
             raise QAPISemError(
                 self.info,
                 "discriminator member '%s' of %s must not be optional"
                 % (self._tag_name, base))
-        if self.tag_member.ifcond.is_present():
+        if tag_member.ifcond.is_present():
             raise QAPISemError(
                 self.info,
                 "discriminator member '%s' of %s must not be conditional"
                 % (self._tag_name, base))
         # branches that are not explicitly covered get an empty type
-        assert self.tag_member.defined_in
+        assert tag_member.defined_in
         cases = {v.name for v in self.variants}
-        for m in self.tag_member.type.members:
+        for m in tag_member.type.members:
             if m.name not in cases:
                 v = QAPISchemaVariant(m.name, self.info,
                                       'q_empty', m.ifcond)
-                v.set_defined_in(self.tag_member.defined_in)
+                v.set_defined_in(tag_member.defined_in)
                 self.variants.append(v)
         if not self.variants:
             raise QAPISemError(self.info, "union has no branches")
@@ -807,11 +799,11 @@ def check(
             # Union names must match enum values; alternate names are
             # checked separately. Use 'seen' to tell the two apart.
             if seen:
-                if v.name not in self.tag_member.type.member_names():
+                if v.name not in tag_member.type.member_names():
                     raise QAPISemError(
                         self.info,
                         "branch '%s' is not a value of %s"
-                        % (v.name, self.tag_member.type.describe()))
+                        % (v.name, tag_member.type.describe()))
                 if not isinstance(v.type, QAPISchemaObjectType):
                     raise QAPISemError(
                         self.info,
@@ -839,7 +831,7 @@ def __init__(self,
                  variants: List[QAPISchemaVariant],
                  tag_member: QAPISchemaObjectTypeMember):
         super().__init__(info, variants)
-        self._tag_member = tag_member
+        self.tag_member = tag_member
 
     def check(
             self, schema: QAPISchema, seen: Dict[str, QAPISchemaMember]
-- 
2.44.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-03-20  7:45 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-20  7:43 [PATCH 0/7] qapi: Refactor QAPISchemaVariants Markus Armbruster
2024-03-20  7:43 ` [PATCH 1/7] qapi: New QAPISchemaBranches, QAPISchemaAlternatives Markus Armbruster
2024-03-20  7:43 ` [PATCH 2/7] qapi: Rename visitor parameter @variants to @branches Markus Armbruster
2024-03-20  7:43 ` [PATCH 3/7] qapi: Rename visitor parameter @variants to @alternatives Markus Armbruster
2024-03-20  7:43 ` [PATCH 4/7] qapi: Rename QAPISchemaObjectType.variants to .branches Markus Armbruster
2024-03-20  7:43 ` [PATCH 5/7] qapi: Rename QAPISchemaAlternateType.variants to .alternatives Markus Armbruster
2024-03-20  7:43 ` [PATCH 6/7] qapi: Move conditional code from QAPISchemaVariants to its subtypes Markus Armbruster
2024-03-20  7:43 ` [PATCH 7/7] qapi: Simplify QAPISchemaVariants @tag_member Markus Armbruster

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).