Re: [Qemu-devel] [PATCH v6 21/27] qapi: add #if conditions to generated code members
On Mon, Dec 10, 2018 at 2:11 PM Markus Armbruster wrote: > > Markus Armbruster writes: > > > Marc-André Lureau writes: > > > >> Hi > >> On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster wrote: > >>> > >>> Marc-André Lureau writes: > >>> > >>> > Wrap generated enum/struct members and code with #if/#endif, using the > >>> > >>> enum and struct members > >> > >> ok > >> > >>> > >>> > .ifcond members added in the previous patches. > >>> > > >>> > Some types generate both enum and struct members for example, so a > >>> > step-by-step is unnecessarily complicated to deal with (it would > >>> > easily generate invalid intermediary code). > >>> > >>> Can you give an example of a schema definition that would lead to > >>> complications? > >>> > >> > >> Honestly, I don't remember well (it's been a while I wrote that code). > > > > I know... > > > >> It must be related to implicit enums, such as union kind... If there > >> is no strong need to split this patch, I would rather not do that > >> extra work. > > > > I'm not looking for reasons to split this patch, I'm looking for > > stronger reasons to keep it just like it is :) > > > > Your hunch that complications would arise for simple unions plausible: > > there the same conditional needs to be applied both to the C enum's > > member and the C union member. > > > > For the generated C code to compile, each union tag enum member > > conditional must imply the corresponding variant conditional. > > > > For flat unions, the two are separate. The QAPI generator makes no > > effort to check the enum member's if condition implies the union > > variant's if condition; if you mess them up in the schema, you get to > > deal with the C compilation errors. > > > > For simple unions, the two are one. > > > > If we separate the generator updates for enums and for union members, > > and do enum members first, then unions with conditional tag members > > can't compile. Corrollary: simple unions with conditional variants > > can't compile. > > > > What if we do union members first? > > > > Again, I'm not asking for patch splitting here, I'm just trying to > > arrive at a clearer understanding to avoid making insufficiently > > supported claims in the commit message. The combined patch looks small > > and clean enough to keep it combined. > > > > [...] > > What about this commit message: > > qapi: Add #if conditions to generated code members > > Wrap generated enum and struct members and their supporting code with > #if/#endif, using the .ifcond members added in the previous patches. > > We do enum and struct in a single patch because union tag enum and the > associated variants tie them together, and dealing with that to split > the patch doesn't seem worthwhile. ack, thanks
Re: [Qemu-devel] [PATCH v6 21/27] qapi: add #if conditions to generated code members
On Mon, Dec 10, 2018 at 2:11 PM Markus Armbruster wrote: > > Markus Armbruster writes: > > > Marc-André Lureau writes: > > > >> Hi > >> On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster wrote: > >>> > >>> Marc-André Lureau writes: > >>> > >>> > Wrap generated enum/struct members and code with #if/#endif, using the > >>> > >>> enum and struct members > >> > >> ok > >> > >>> > >>> > .ifcond members added in the previous patches. > >>> > > >>> > Some types generate both enum and struct members for example, so a > >>> > step-by-step is unnecessarily complicated to deal with (it would > >>> > easily generate invalid intermediary code). > >>> > >>> Can you give an example of a schema definition that would lead to > >>> complications? > >>> > >> > >> Honestly, I don't remember well (it's been a while I wrote that code). > > > > I know... > > > >> It must be related to implicit enums, such as union kind... If there > >> is no strong need to split this patch, I would rather not do that > >> extra work. > > > > I'm not looking for reasons to split this patch, I'm looking for > > stronger reasons to keep it just like it is :) > > > > Your hunch that complications would arise for simple unions plausible: > > there the same conditional needs to be applied both to the C enum's > > member and the C union member. > > > > For the generated C code to compile, each union tag enum member > > conditional must imply the corresponding variant conditional. > > > > For flat unions, the two are separate. The QAPI generator makes no > > effort to check the enum member's if condition implies the union > > variant's if condition; if you mess them up in the schema, you get to > > deal with the C compilation errors. > > > > For simple unions, the two are one. > > > > If we separate the generator updates for enums and for union members, > > and do enum members first, then unions with conditional tag members > > can't compile. Corrollary: simple unions with conditional variants > > can't compile. > > > > What if we do union members first? > > > > Again, I'm not asking for patch splitting here, I'm just trying to > > arrive at a clearer understanding to avoid making insufficiently > > supported claims in the commit message. The combined patch looks small > > and clean enough to keep it combined. > > > > [...] > > What about this commit message: > > qapi: Add #if conditions to generated code members > > Wrap generated enum and struct members and their supporting code with > #if/#endif, using the .ifcond members added in the previous patches. > > We do enum and struct in a single patch because union tag enum and the > associated variants tie them together, and dealing with that to split > the patch doesn't seem worthwhile. > ack, thanks -- Marc-André Lureau
Re: [Qemu-devel] [PATCH v6 21/27] qapi: add #if conditions to generated code members
Markus Armbruster writes: > Marc-André Lureau writes: > >> Hi >> On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster wrote: >>> >>> Marc-André Lureau writes: >>> >>> > Wrap generated enum/struct members and code with #if/#endif, using the >>> >>> enum and struct members >> >> ok >> >>> >>> > .ifcond members added in the previous patches. >>> > >>> > Some types generate both enum and struct members for example, so a >>> > step-by-step is unnecessarily complicated to deal with (it would >>> > easily generate invalid intermediary code). >>> >>> Can you give an example of a schema definition that would lead to >>> complications? >>> >> >> Honestly, I don't remember well (it's been a while I wrote that code). > > I know... > >> It must be related to implicit enums, such as union kind... If there >> is no strong need to split this patch, I would rather not do that >> extra work. > > I'm not looking for reasons to split this patch, I'm looking for > stronger reasons to keep it just like it is :) > > Your hunch that complications would arise for simple unions plausible: > there the same conditional needs to be applied both to the C enum's > member and the C union member. > > For the generated C code to compile, each union tag enum member > conditional must imply the corresponding variant conditional. > > For flat unions, the two are separate. The QAPI generator makes no > effort to check the enum member's if condition implies the union > variant's if condition; if you mess them up in the schema, you get to > deal with the C compilation errors. > > For simple unions, the two are one. > > If we separate the generator updates for enums and for union members, > and do enum members first, then unions with conditional tag members > can't compile. Corrollary: simple unions with conditional variants > can't compile. > > What if we do union members first? > > Again, I'm not asking for patch splitting here, I'm just trying to > arrive at a clearer understanding to avoid making insufficiently > supported claims in the commit message. The combined patch looks small > and clean enough to keep it combined. > > [...] What about this commit message: qapi: Add #if conditions to generated code members Wrap generated enum and struct members and their supporting code with #if/#endif, using the .ifcond members added in the previous patches. We do enum and struct in a single patch because union tag enum and the associated variants tie them together, and dealing with that to split the patch doesn't seem worthwhile.
Re: [Qemu-devel] [PATCH v6 21/27] qapi: add #if conditions to generated code members
Marc-André Lureau writes: > Hi > On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster wrote: >> >> Marc-André Lureau writes: >> >> > Wrap generated enum/struct members and code with #if/#endif, using the >> >> enum and struct members > > ok > >> >> > .ifcond members added in the previous patches. >> > >> > Some types generate both enum and struct members for example, so a >> > step-by-step is unnecessarily complicated to deal with (it would >> > easily generate invalid intermediary code). >> >> Can you give an example of a schema definition that would lead to >> complications? >> > > Honestly, I don't remember well (it's been a while I wrote that code). I know... > It must be related to implicit enums, such as union kind... If there > is no strong need to split this patch, I would rather not do that > extra work. I'm not looking for reasons to split this patch, I'm looking for stronger reasons to keep it just like it is :) Your hunch that complications would arise for simple unions plausible: there the same conditional needs to be applied both to the C enum's member and the C union member. For the generated C code to compile, each union tag enum member conditional must imply the corresponding variant conditional. For flat unions, the two are separate. The QAPI generator makes no effort to check the enum member's if condition implies the union variant's if condition; if you mess them up in the schema, you get to deal with the C compilation errors. For simple unions, the two are one. If we separate the generator updates for enums and for union members, and do enum members first, then unions with conditional tag members can't compile. Corrollary: simple unions with conditional variants can't compile. What if we do union members first? Again, I'm not asking for patch splitting here, I'm just trying to arrive at a clearer understanding to avoid making insufficiently supported claims in the commit message. The combined patch looks small and clean enough to keep it combined. [...]
Re: [Qemu-devel] [PATCH v6 21/27] qapi: add #if conditions to generated code members
Hi
On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster wrote:
>
> Marc-André Lureau writes:
>
> > Wrap generated enum/struct members and code with #if/#endif, using the
>
> enum and struct members
ok
>
> > .ifcond members added in the previous patches.
> >
> > Some types generate both enum and struct members for example, so a
> > step-by-step is unnecessarily complicated to deal with (it would
> > easily generate invalid intermediary code).
>
> Can you give an example of a schema definition that would lead to
> complications?
>
Honestly, I don't remember well (it's been a while I wrote that code).
It must be related to implicit enums, such as union kind... If there
is no strong need to split this patch, I would rather not do that
extra work.
> >
> > Signed-off-by: Marc-André Lureau
> > ---
> > scripts/qapi/common.py | 4
> > scripts/qapi/introspect.py | 13 +
> > scripts/qapi/types.py | 4
> > scripts/qapi/visit.py | 6 ++
> > 4 files changed, 23 insertions(+), 4 deletions(-)
> >
> > diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> > index b3b64a60bf..a66c035b91 100644
> > --- a/scripts/qapi/common.py
> > +++ b/scripts/qapi/common.py
> > @@ -2098,11 +2098,13 @@ const QEnumLookup %(c_name)s_lookup = {
> > ''',
> > c_name=c_name(name))
> > for m in members:
> > +ret += gen_if(m.ifcond)
> > index = c_enum_const(name, m.name, prefix)
> > ret += mcgen('''
> > [%(index)s] = "%(name)s",
> > ''',
> > index=index, name=m.name)
> > +ret += gen_endif(m.ifcond)
> >
> > ret += mcgen('''
> > },
> > @@ -2124,10 +2126,12 @@ typedef enum %(c_name)s {
> > c_name=c_name(name))
> >
> > for m in enum_members:
> > +ret += gen_if(m.ifcond)
> > ret += mcgen('''
> > %(c_enum)s,
> > ''',
> > c_enum=c_enum_const(name, m.name, prefix))
> > +ret += gen_endif(m.ifcond)
> >
> > ret += mcgen('''
> > } %(c_name)s;
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index 3f1ca99f6d..bf5db51f4a 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -148,6 +148,8 @@ const QLitObject %(c_name)s = %(c_string)s;
> > ret = {'name': member.name, 'type': self._use_type(member.type)}
> > if member.optional:
> > ret['default'] = None
> > +if member.ifcond:
> > +ret = (ret, member.ifcond)
> > return ret
> >
> > def _gen_variants(self, tag_name, variants):
> > @@ -155,14 +157,16 @@ const QLitObject %(c_name)s = %(c_string)s;
> > 'variants': [self._gen_variant(v) for v in variants]}
> >
> > def _gen_variant(self, variant):
> > -return {'case': variant.name, 'type': self._use_type(variant.type)}
> > +return ({'case': variant.name, 'type':
> > self._use_type(variant.type)},
> > +variant.ifcond)
>
> Looks different in your rebased version at
> https://github.com/elmarco/qemu.git branch qapi-if. I'm only skimming
> this version.
>
> Note to self: always creates the tuple form, even when there's no
> if-condition. Differs from _gen_qlit(), which creates a tuple only when
> there's a if-condition. Not wrong, just a bit inconsistent.
>
> >
> > def visit_builtin_type(self, name, info, json_type):
> > self._gen_qlit(name, 'builtin', {'json-type': json_type}, [])
> >
> > def visit_enum_type(self, name, info, ifcond, members, prefix):
> > self._gen_qlit(name, 'enum',
> > - {'values': [m.name for m in members]}, ifcond)
> > + {'values': [(m.name, m.ifcond) for m in members]},
> > + ifcond)
>
> Likewise.
>
> >
> > def visit_array_type(self, name, info, ifcond, element_type):
> > element = self._use_type(element_type)
> > @@ -178,8 +182,9 @@ const QLitObject %(c_name)s = %(c_string)s;
> >
> > def visit_alternate_type(self, name, info, ifcond, variants):
> > self._gen_qlit(name, 'alternate',
> > - {'members': [{'type': self._use_type(m.type)}
> > -for m in variants.variants]}, ifcond)
> > + {'members': [
> > + ({'type': self._use_type(m.type)}, m.ifcond)
> > + for m in variants.variants]}, ifcond)
>
> Likewise.
>
> >
> > def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
> >success_response, boxed, allow_oob, allow_preconfig):
> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> > index 2d4a70f810..7d9eef6320 100644
> > --- a/scripts/qapi/types.py
> > +++ b/scripts/qapi/types.py
> > @@ -43,6 +43,7 @@ struct %(c_name)s {
> > def gen_struct_members(members):
> > ret = ''
> > for memb in members:
> > +ret += gen_if(memb.ifcond)
Re: [Qemu-devel] [PATCH v6 21/27] qapi: add #if conditions to generated code members
Marc-André Lureau writes:
> Wrap generated enum/struct members and code with #if/#endif, using the
enum and struct members
> .ifcond members added in the previous patches.
>
> Some types generate both enum and struct members for example, so a
> step-by-step is unnecessarily complicated to deal with (it would
> easily generate invalid intermediary code).
Can you give an example of a schema definition that would lead to
complications?
>
> Signed-off-by: Marc-André Lureau
> ---
> scripts/qapi/common.py | 4
> scripts/qapi/introspect.py | 13 +
> scripts/qapi/types.py | 4
> scripts/qapi/visit.py | 6 ++
> 4 files changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
> index b3b64a60bf..a66c035b91 100644
> --- a/scripts/qapi/common.py
> +++ b/scripts/qapi/common.py
> @@ -2098,11 +2098,13 @@ const QEnumLookup %(c_name)s_lookup = {
> ''',
> c_name=c_name(name))
> for m in members:
> +ret += gen_if(m.ifcond)
> index = c_enum_const(name, m.name, prefix)
> ret += mcgen('''
> [%(index)s] = "%(name)s",
> ''',
> index=index, name=m.name)
> +ret += gen_endif(m.ifcond)
>
> ret += mcgen('''
> },
> @@ -2124,10 +2126,12 @@ typedef enum %(c_name)s {
> c_name=c_name(name))
>
> for m in enum_members:
> +ret += gen_if(m.ifcond)
> ret += mcgen('''
> %(c_enum)s,
> ''',
> c_enum=c_enum_const(name, m.name, prefix))
> +ret += gen_endif(m.ifcond)
>
> ret += mcgen('''
> } %(c_name)s;
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 3f1ca99f6d..bf5db51f4a 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -148,6 +148,8 @@ const QLitObject %(c_name)s = %(c_string)s;
> ret = {'name': member.name, 'type': self._use_type(member.type)}
> if member.optional:
> ret['default'] = None
> +if member.ifcond:
> +ret = (ret, member.ifcond)
> return ret
>
> def _gen_variants(self, tag_name, variants):
> @@ -155,14 +157,16 @@ const QLitObject %(c_name)s = %(c_string)s;
> 'variants': [self._gen_variant(v) for v in variants]}
>
> def _gen_variant(self, variant):
> -return {'case': variant.name, 'type': self._use_type(variant.type)}
> +return ({'case': variant.name, 'type': self._use_type(variant.type)},
> +variant.ifcond)
Looks different in your rebased version at
https://github.com/elmarco/qemu.git branch qapi-if. I'm only skimming
this version.
Note to self: always creates the tuple form, even when there's no
if-condition. Differs from _gen_qlit(), which creates a tuple only when
there's a if-condition. Not wrong, just a bit inconsistent.
>
> def visit_builtin_type(self, name, info, json_type):
> self._gen_qlit(name, 'builtin', {'json-type': json_type}, [])
>
> def visit_enum_type(self, name, info, ifcond, members, prefix):
> self._gen_qlit(name, 'enum',
> - {'values': [m.name for m in members]}, ifcond)
> + {'values': [(m.name, m.ifcond) for m in members]},
> + ifcond)
Likewise.
>
> def visit_array_type(self, name, info, ifcond, element_type):
> element = self._use_type(element_type)
> @@ -178,8 +182,9 @@ const QLitObject %(c_name)s = %(c_string)s;
>
> def visit_alternate_type(self, name, info, ifcond, variants):
> self._gen_qlit(name, 'alternate',
> - {'members': [{'type': self._use_type(m.type)}
> -for m in variants.variants]}, ifcond)
> + {'members': [
> + ({'type': self._use_type(m.type)}, m.ifcond)
> + for m in variants.variants]}, ifcond)
Likewise.
>
> def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
>success_response, boxed, allow_oob, allow_preconfig):
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> index 2d4a70f810..7d9eef6320 100644
> --- a/scripts/qapi/types.py
> +++ b/scripts/qapi/types.py
> @@ -43,6 +43,7 @@ struct %(c_name)s {
> def gen_struct_members(members):
> ret = ''
> for memb in members:
> +ret += gen_if(memb.ifcond)
> if memb.optional:
> ret += mcgen('''
> bool has_%(c_name)s;
> @@ -52,6 +53,7 @@ def gen_struct_members(members):
> %(c_type)s %(c_name)s;
> ''',
> c_type=memb.type.c_type(), c_name=c_name(memb.name))
> +ret += gen_endif(memb.ifcond)
> return ret
>
>
> @@ -131,11 +133,13 @@ def gen_variants(variants):
> for var in variants.variants:
> if var.type.name == 'q_empty':
> continue
> +ret +=
[Qemu-devel] [PATCH v6 21/27] qapi: add #if conditions to generated code members
Wrap generated enum/struct members and code with #if/#endif, using the
.ifcond members added in the previous patches.
Some types generate both enum and struct members for example, so a
step-by-step is unnecessarily complicated to deal with (it would
easily generate invalid intermediary code).
Signed-off-by: Marc-André Lureau
---
scripts/qapi/common.py | 4
scripts/qapi/introspect.py | 13 +
scripts/qapi/types.py | 4
scripts/qapi/visit.py | 6 ++
4 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index b3b64a60bf..a66c035b91 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -2098,11 +2098,13 @@ const QEnumLookup %(c_name)s_lookup = {
''',
c_name=c_name(name))
for m in members:
+ret += gen_if(m.ifcond)
index = c_enum_const(name, m.name, prefix)
ret += mcgen('''
[%(index)s] = "%(name)s",
''',
index=index, name=m.name)
+ret += gen_endif(m.ifcond)
ret += mcgen('''
},
@@ -2124,10 +2126,12 @@ typedef enum %(c_name)s {
c_name=c_name(name))
for m in enum_members:
+ret += gen_if(m.ifcond)
ret += mcgen('''
%(c_enum)s,
''',
c_enum=c_enum_const(name, m.name, prefix))
+ret += gen_endif(m.ifcond)
ret += mcgen('''
} %(c_name)s;
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 3f1ca99f6d..bf5db51f4a 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -148,6 +148,8 @@ const QLitObject %(c_name)s = %(c_string)s;
ret = {'name': member.name, 'type': self._use_type(member.type)}
if member.optional:
ret['default'] = None
+if member.ifcond:
+ret = (ret, member.ifcond)
return ret
def _gen_variants(self, tag_name, variants):
@@ -155,14 +157,16 @@ const QLitObject %(c_name)s = %(c_string)s;
'variants': [self._gen_variant(v) for v in variants]}
def _gen_variant(self, variant):
-return {'case': variant.name, 'type': self._use_type(variant.type)}
+return ({'case': variant.name, 'type': self._use_type(variant.type)},
+variant.ifcond)
def visit_builtin_type(self, name, info, json_type):
self._gen_qlit(name, 'builtin', {'json-type': json_type}, [])
def visit_enum_type(self, name, info, ifcond, members, prefix):
self._gen_qlit(name, 'enum',
- {'values': [m.name for m in members]}, ifcond)
+ {'values': [(m.name, m.ifcond) for m in members]},
+ ifcond)
def visit_array_type(self, name, info, ifcond, element_type):
element = self._use_type(element_type)
@@ -178,8 +182,9 @@ const QLitObject %(c_name)s = %(c_string)s;
def visit_alternate_type(self, name, info, ifcond, variants):
self._gen_qlit(name, 'alternate',
- {'members': [{'type': self._use_type(m.type)}
-for m in variants.variants]}, ifcond)
+ {'members': [
+ ({'type': self._use_type(m.type)}, m.ifcond)
+ for m in variants.variants]}, ifcond)
def visit_command(self, name, info, ifcond, arg_type, ret_type, gen,
success_response, boxed, allow_oob, allow_preconfig):
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
index 2d4a70f810..7d9eef6320 100644
--- a/scripts/qapi/types.py
+++ b/scripts/qapi/types.py
@@ -43,6 +43,7 @@ struct %(c_name)s {
def gen_struct_members(members):
ret = ''
for memb in members:
+ret += gen_if(memb.ifcond)
if memb.optional:
ret += mcgen('''
bool has_%(c_name)s;
@@ -52,6 +53,7 @@ def gen_struct_members(members):
%(c_type)s %(c_name)s;
''',
c_type=memb.type.c_type(), c_name=c_name(memb.name))
+ret += gen_endif(memb.ifcond)
return ret
@@ -131,11 +133,13 @@ def gen_variants(variants):
for var in variants.variants:
if var.type.name == 'q_empty':
continue
+ret += gen_if(var.ifcond)
ret += mcgen('''
%(c_type)s %(c_name)s;
''',
c_type=var.type.c_unboxed_type(),
c_name=c_name(var.name))
+ret += gen_endif(var.ifcond)
ret += mcgen('''
} u;
diff --git a/scripts/qapi/visit.py b/scripts/qapi/visit.py
index 24f85a2e85..82eab72b21 100644
--- a/scripts/qapi/visit.py
+++ b/scripts/qapi/visit.py
@@ -54,6 +54,7 @@ void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s
*obj, Error **errp)
c_type=base.c_name())
for memb in members:
+ret += gen_if(memb.ifcond)
if memb.optional:
ret += mcgen('''
if (visit_optional(v, "%(name)s", >has_%(c_name)s))
