Re: [Qemu-devel] [PATCH v4 1/2] qapi: allow empty branches in flat unions
Anton Nefedov writes: > On 14/6/2018 10:19 AM, Markus Armbruster wrote: >> Anton Nefedov writes: >> >>> It often happens that just a few discriminator values imply extra data in >>> a flat union. Existing checks did not make possible to leave other values >>> uncovered. Such cases had to be worked around by either stating a dummy >>> (empty) type or introducing another (subset) discriminator enumeration. >>> >>> Both options create redundant entities in qapi files for little profit. >>> >>> With this patch it is not necessary anymore to add designated union >>> fields for every possible value of a discriminator enumeration. >>> >>> Signed-off-by: Anton Nefedov >>> Reviewed-by: Eric Blake [...] >>> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py >>> index 2462fc0..0f3d89b 100644 >>> --- a/scripts/qapi/common.py >>> +++ b/scripts/qapi/common.py >>> @@ -779,13 +779,6 @@ def check_union(expr, info): >>> "enum '%s'" >>> % (key, enum_define['enum'])) >>> -# If discriminator is user-defined, ensure all values are >>> covered >>> -if enum_define: >>> -for value in enum_define['data']: >>> -if value not in members.keys(): >>> -raise QAPISemError(info, "Union '%s' data missing '%s' >>> branch" >>> - % (name, value)) >>> - >>> def check_alternate(expr, info): >>> name = expr['alternate'] >>> @@ -1644,6 +1637,10 @@ class QAPISchema(object): >>> if tag_name: >>> variants = [self._make_variant(key, value) >>> for (key, value) in data.items()] >>> +# branches that are not explicitly covered get an empty type >>> +variants += [self._make_variant(key, 'q_empty') >>> + for key in >>> discriminator_find_enum_define(expr)['data'] >> >> Long line, flagged by pycodestyle --diff. >> > > Done. > >>> + if key not in data.keys()] >>> members = [] >>> else: >>> variants = [self._make_simple_variant(key, value, info) >> >> The way you desugar the union is simple enough, but it uses >> discriminator_find_enum_define() outside check_exprs(), which I'd rather >> avoid. Not something you could be reasonably expected to know. >> >> We could desugar in QAPISchemaObjectTypeVariants.check(), where we have >> self.tag_member.type.values. >> >> @@ -1346,10 +1346,17 @@ class QAPISchemaObjectTypeVariants(object): >> v.set_owner(name) >> >> def check(self, schema, seen): >> -if not self.tag_member:# flat union >> +if not self.tag_member: # flat union >> self.tag_member = seen[c_name(self._tag_name)] >> assert self._tag_name == self.tag_member.name >> assert isinstance(self.tag_member.type, QAPISchemaEnumType) >> +if self._tag_name: # flat union >> +cases = set([v.name for v in self.variants]) >> +for val in self.tag_member.type.values: >> +if val.name not in cases: >> +v = QAPISchemaObjectTypeVariant(val.name, 'q_empty') >> +v.set_owner(self.tag_member.owner) >> +self.variants.append(v) >> for v in self.variants: >> v.check(schema) >> # Union names must match enum values; alternate names are >> >> Not so simple anymore. >> >> Simpler: desugar in check_union(), i.e. replace your two hunks by just >> >> @@ -783,8 +783,7 @@ def check_union(expr, info): >> if enum_define: >> for value in enum_define['data']: >> if value not in members.keys(): >> -raise QAPISemError(info, "Union '%s' data missing '%s' >> branch" >> - % (name, value)) >> +members[value] = 'q_empty' >> >> >> def check_alternate(expr, info): >> >> What do you think? >> > > it didn't feel right aesthetically to normalize by modifying the parsed > expressions especially at check_exprs() stage. But maybe using > discriminator_find_enum_define() is more sloppy, I can't quite see The issue isn't sloppiness, it's adding to our technical debt. To explain this, I need to relate a bit of history. In the beginning, we did everything with syntax trees. Commit ac88219a6c7 (Sep 2015) spliced in an intermediate representation. Its commit message explains: The QAPI code generators work with a syntax tree (nested dictionaries) plus a few symbol tables (also dictionaries) on the side. They have clearly outgrown these simple data structures. There's lots of rummaging around in dictionaries, and information is recomputed on the fly. For the work I'm going to do, I want more clearly defined and more convenient interfaces.
Re: [Qemu-devel] [PATCH v4 1/2] qapi: allow empty branches in flat unions
On 14/6/2018 10:19 AM, Markus Armbruster wrote:
Anton Nefedov writes:
It often happens that just a few discriminator values imply extra data in
a flat union. Existing checks did not make possible to leave other values
uncovered. Such cases had to be worked around by either stating a dummy
(empty) type or introducing another (subset) discriminator enumeration.
Both options create redundant entities in qapi files for little profit.
With this patch it is not necessary anymore to add designated union
fields for every possible value of a discriminator enumeration.
Signed-off-by: Anton Nefedov
Reviewed-by: Eric Blake
---
docs/devel/qapi-code-gen.txt| 8 +---
tests/qapi-schema/flat-union-incomplete-branch.json | 9 -
tests/qapi-schema/qapi-schema-test.json | 4 ++--
scripts/qapi/common.py | 11 ---
scripts/qapi/types.py | 4 +++-
scripts/qapi/visit.py | 17 +
tests/Makefile.include | 1 -
tests/qapi-schema/flat-union-incomplete-branch.err | 1 -
tests/qapi-schema/flat-union-incomplete-branch.exit | 1 -
tests/qapi-schema/flat-union-incomplete-branch.out | 0
tests/qapi-schema/qapi-schema-test.out | 3 ++-
11 files changed, 29 insertions(+), 30 deletions(-)
delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.json
delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.err
delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.exit
delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.out
diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index 1366228..88a70e4 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -496,9 +496,11 @@ Resulting in these JSON objects:
Notice that in a flat union, the discriminator name is controlled by
the user, but because it must map to a base member with enum type, the
-code generator can ensure that branches exist for all values of the
-enum (although the order of the keys need not match the declaration of
-the enum). In the resulting generated C data types, a flat union is
+code generator ensures that branches match the existing values of the
+enum. The order of the keys need not match the declaration of the enum.
+The keys need not cover all possible enum values. Omitted enum values
+are still valid branches that add no additional members to the data type.
+In the resulting generated C data types, a flat union is
represented as a struct with the base members included directly, and
then a union of structures for each branch of the struct.
diff --git a/tests/qapi-schema/flat-union-incomplete-branch.json b/tests/qapi-schema/flat-union-incomplete-branch.json
deleted file mode 100644
index 25a411b..000
--- a/tests/qapi-schema/flat-union-incomplete-branch.json
+++ /dev/null
@@ -1,9 +0,0 @@
-# we require all branches of the union to be covered
-{ 'enum': 'TestEnum',
- 'data': [ 'value1', 'value2' ] }
-{ 'struct': 'TestTypeA',
- 'data': { 'string': 'str' } }
-{ 'union': 'TestUnion',
- 'base': { 'type': 'TestEnum' },
- 'discriminator': 'type',
- 'data': { 'value1': 'TestTypeA' } }
diff --git a/tests/qapi-schema/qapi-schema-test.json
b/tests/qapi-schema/qapi-schema-test.json
index 46c7282..881b53b 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -39,7 +39,7 @@
'*enum1': 'EnumOne' } } # intentional forward reference
{ 'enum': 'EnumOne',
- 'data': [ 'value1', 'value2', 'value3' ] }
+ 'data': [ 'value1', 'value2', 'value3', 'value4' ] }
{ 'struct': 'UserDefZero',
'data': { 'integer': 'int' } }
@@ -76,7 +76,7 @@
'discriminator': 'enum1',
'data': { 'value1' : 'UserDefA',
'value2' : 'UserDefB',
-'value3' : 'UserDefB' } }
+'value3' : 'UserDefB' } } # skip 'value4'
Perhaps
'value3' : 'UserDefB'
# 'value4' defaults to empty
} }
would be clearer.
Sure
{ 'struct': 'UserDefUnionBase',
'base': 'UserDefZero',
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index 2462fc0..0f3d89b 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -779,13 +779,6 @@ def check_union(expr, info):
"enum '%s'"
% (key, enum_define['enum']))
-# If discriminator is user-defined, ensure all values are covered
-if enum_define:
-for value in enum_define['data']:
-if value not in members.keys():
-raise QAPISemError(info, "Union '%s' data missing '%s' branch"
- % (name, value))
-
def check_alternate(expr, info):
name = expr['alternate']
@@ -1644,6 +1637,10 @@ class
Re: [Qemu-devel] [PATCH v4 1/2] qapi: allow empty branches in flat unions
Anton Nefedov writes:
> It often happens that just a few discriminator values imply extra data in
> a flat union. Existing checks did not make possible to leave other values
> uncovered. Such cases had to be worked around by either stating a dummy
> (empty) type or introducing another (subset) discriminator enumeration.
>
> Both options create redundant entities in qapi files for little profit.
>
> With this patch it is not necessary anymore to add designated union
> fields for every possible value of a discriminator enumeration.
>
> Signed-off-by: Anton Nefedov
> Reviewed-by: Eric Blake
> ---
> docs/devel/qapi-code-gen.txt| 8 +---
> tests/qapi-schema/flat-union-incomplete-branch.json | 9 -
> tests/qapi-schema/qapi-schema-test.json | 4 ++--
> scripts/qapi/common.py | 11 ---
> scripts/qapi/types.py | 4 +++-
> scripts/qapi/visit.py | 17 +
> tests/Makefile.include | 1 -
> tests/qapi-schema/flat-union-incomplete-branch.err | 1 -
> tests/qapi-schema/flat-union-incomplete-branch.exit | 1 -
> tests/qapi-schema/flat-union-incomplete-branch.out | 0
> tests/qapi-schema/qapi-schema-test.out | 3 ++-
> 11 files changed, 29 insertions(+), 30 deletions(-)
> delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.json
> delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.err
> delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.exit
> delete mode 100644 tests/qapi-schema/flat-union-incomplete-branch.out
>
> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> index 1366228..88a70e4 100644
> --- a/docs/devel/qapi-code-gen.txt
> +++ b/docs/devel/qapi-code-gen.txt
> @@ -496,9 +496,11 @@ Resulting in these JSON objects:
>
> Notice that in a flat union, the discriminator name is controlled by
> the user, but because it must map to a base member with enum type, the
> -code generator can ensure that branches exist for all values of the
> -enum (although the order of the keys need not match the declaration of
> -the enum). In the resulting generated C data types, a flat union is
> +code generator ensures that branches match the existing values of the
> +enum. The order of the keys need not match the declaration of the enum.
> +The keys need not cover all possible enum values. Omitted enum values
> +are still valid branches that add no additional members to the data type.
> +In the resulting generated C data types, a flat union is
> represented as a struct with the base members included directly, and
> then a union of structures for each branch of the struct.
>
> diff --git a/tests/qapi-schema/flat-union-incomplete-branch.json
> b/tests/qapi-schema/flat-union-incomplete-branch.json
> deleted file mode 100644
> index 25a411b..000
> --- a/tests/qapi-schema/flat-union-incomplete-branch.json
> +++ /dev/null
> @@ -1,9 +0,0 @@
> -# we require all branches of the union to be covered
> -{ 'enum': 'TestEnum',
> - 'data': [ 'value1', 'value2' ] }
> -{ 'struct': 'TestTypeA',
> - 'data': { 'string': 'str' } }
> -{ 'union': 'TestUnion',
> - 'base': { 'type': 'TestEnum' },
> - 'discriminator': 'type',
> - 'data': { 'value1': 'TestTypeA' } }
> diff --git a/tests/qapi-schema/qapi-schema-test.json
> b/tests/qapi-schema/qapi-schema-test.json
> index 46c7282..881b53b 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -39,7 +39,7 @@
> '*enum1': 'EnumOne' } } # intentional forward reference
>
> { 'enum': 'EnumOne',
> - 'data': [ 'value1', 'value2', 'value3' ] }
> + 'data': [ 'value1', 'value2', 'value3', 'value4' ] }
>
> { 'struct': 'UserDefZero',
>'data': { 'integer': 'int' } }
> @@ -76,7 +76,7 @@
>'discriminator': 'enum1',
>'data': { 'value1' : 'UserDefA',
> 'value2' : 'UserDefB',
> -'value3' : 'UserDefB' } }
> +'value3' : 'UserDefB' } } # skip 'value4'
Perhaps
'value3' : 'UserDefB'
# 'value4' defaults to empty
} }
would be clearer.
>
> { 'struct': 'UserDefUnionBase',
>'base': 'UserDefZero',
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index 2462fc0..0f3d89b 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -779,13 +779,6 @@ def check_union(expr, info):
> "enum '%s'"
> % (key, enum_define['enum']))
>
> -# If discriminator is user-defined, ensure all values are covered
> -if enum_define:
> -for value in enum_define['data']:
> -if value not in members.keys():
> -raise QAPISemError(info, "Union '%s' data missing '%s'
> branch"
> - % (name, value))
> -
>
> def
