Re: [PATCH v3 2/3] qapi: Do not generate empty enum
Eric Blake writes:
> On Tue, Mar 21, 2023 at 03:19:28PM +, Daniel P. Berrangé wrote:
>> On Tue, Mar 21, 2023 at 03:31:56PM +0100, Philippe Mathieu-Daudé wrote:
>> > On 16/3/23 15:57, Markus Armbruster wrote:
>> > > Daniel P. Berrangé writes:
>> > >
>> > > > On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:
>> > > > > Philippe Mathieu-Daudé writes:
>> > > > >
>> > > > > > Per the C++ standard, empty enum are ill-formed. Do not generate
>> > >
>> > > The C standard. The C++ standard doesn't apply here :)
>> >
>> > I can't find how empty enums are considered by the C standard...
C99 §6.7.2.2 Enumeration specifiers
enum-specifier:
enum identifier-opt { enumerator-list }
enum identifier-opt { enumerator-list , }
enum identifier
enumerator-list:
enumerator
enumerator-list , enumerator
enumerator:
enumeration-constant
enumeration-constant = constant-expr
Empty enum is a syntax error.
>> The C standard doesn't really matter either.
>>
>> What we actually care about is whether GCC and CLang consider the
>> empty enums to be permissible or not. or to put it another way...
>> if it compiles, ship it :-)
>
> But it doesn't compile:
>
> $ cat foo.c
> typedef enum Blah {
> } Blah;
> int main(void) {
> Blah b = 0;
> }
> $ gcc -o foo -Wall foo.c
> foo.c:2:1: error: empty enum is invalid
> 2 | } Blah;
> | ^
Gcc opts for an error message more useful than "identifier expected".
> foo.c: In function ‘main’:
> foo.c:4:5: error: unknown type name ‘Blah’; use ‘enum’ keyword to refer to
> the type
> 4 | Blah b = 0;
> | ^~~~
> | enum
> foo.c:4:10: warning: unused variable ‘b’ [-Wunused-variable]
> 4 | Blah b = 0;
> | ^
>
> So we _do_ need to avoid creating an enum with all members optional in
> the configuration where all options are disabled, if we want that
> configuration to compile. Or require that all QAPI enums have at
> least one non-optional member.
There is just one way to avoid creating such an enum: make sure the QAPI
enum has members in all configurations we care for.
The issue at hand is whether catching empty enums in qapi-gen already is
practical. Desirable, because qapi-gen errors are friendlier than C
compiler errors in generated code.
Note "practical": qapi-gen makes an effort to catch errors before the C
compiler catches them. But catching all of them is impractical.
Having qapi-gen catch empty enums sure is practical for truly empty
enums. But I doubt an enum without any members is a mistake people make
much.
It isn't for enums with only conditional members: the configuration that
makes them all vanish may or may not actually matter, or even exist, and
qapi-gen can't tell. The C compiler can tell, but only for the
configuration being compiled.
Requiring at least one non-optional member is a restriction: enums with
only conditional members are now rejected even when the configuration
that makes them all vanish does not actually matter.
Is shielding the user from C compiler errors about empty enums worth the
restriction?
Re: [PATCH v3 2/3] qapi: Do not generate empty enum
On Tue, Mar 21, 2023 at 03:19:28PM +, Daniel P. Berrangé wrote:
> On Tue, Mar 21, 2023 at 03:31:56PM +0100, Philippe Mathieu-Daudé wrote:
> > On 16/3/23 15:57, Markus Armbruster wrote:
> > > Daniel P. Berrangé writes:
> > >
> > > > On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:
> > > > > Philippe Mathieu-Daudé writes:
> > > > >
> > > > > > Per the C++ standard, empty enum are ill-formed. Do not generate
> > >
> > > The C standard. The C++ standard doesn't apply here :)
> >
> > I can't find how empty enums are considered by the C standard...
>
> The C standard doesn't really matter either.
>
> What we actually care about is whether GCC and CLang consider the
> empty enums to be permissible or not. or to put it another way...
> if it compiles, ship it :-)
But it doesn't compile:
$ cat foo.c
typedef enum Blah {
} Blah;
int main(void) {
Blah b = 0;
}
$ gcc -o foo -Wall foo.c
foo.c:2:1: error: empty enum is invalid
2 | } Blah;
| ^
foo.c: In function ‘main’:
foo.c:4:5: error: unknown type name ‘Blah’; use ‘enum’ keyword to refer to the
type
4 | Blah b = 0;
| ^~~~
| enum
foo.c:4:10: warning: unused variable ‘b’ [-Wunused-variable]
4 | Blah b = 0;
| ^
So we _do_ need to avoid creating an enum with all members optional in
the configuration where all options are disabled, if we want that
configuration to compile. Or require that all QAPI enums have at
least one non-optional member.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Re: [PATCH v3 2/3] qapi: Do not generate empty enum
Philippe Mathieu-Daudé writes:
> Can we meet half-way only generating the MAX definitions for
> unconditional enums, keeping the conditional ones as is?
>
> -- >8 --
> diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
> @@ -88,16 +88,18 @@ def gen_enum(name: str,
> members: List[QAPISchemaEnumMember],
> prefix: Optional[str] = None) -> str:
> assert members
> -# append automatically generated _MAX value
> -enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
> -
> ret = mcgen('''
>
> typedef enum %(c_name)s {
> ''',
> c_name=c_name(name))
>
> -for memb in enum_members:
> +has_cond = any(memb.ifcond.is_present() for memb in members)
> +if has_cond:
> +# append automatically generated _MAX value
> +members += [QAPISchemaEnumMember('_MAX', None)]
> +
> +for memb in members:
> ret += memb.ifcond.gen_if()
> ret += mcgen('''
> %(c_enum)s,
> @@ -105,6 +107,13 @@ def gen_enum(name: str,
> c_enum=c_enum_const(name, memb.name, prefix))
> ret += memb.ifcond.gen_endif()
>
> +if not has_cond:
> +ret += mcgen('''
> +#define %(c_name)s %(c_length)s
> +''',
> + c_name=c_enum_const(name, '_MAX', prefix),
> + c_length=len(members))
> +
> ret += mcgen('''
> } %(c_name)s;
> ''',
> ---
I doubt the benefit "we need a silly case FOO__MAX only sometimes" is
worth the special case.
We could generate something like
#if [last_member's condition]
#define FOO__MAX (FOO_last_member + 1)
#elif [second_to_last_member's condition]
#define FOO__MAX (FOO_second_to_last_member + 1)
...
#else
#define FOO__MAX (FOO_last_unconditional_member + 1)
#endif
but whether that is worth the additional complexity seems doubtful, too.
Re: [PATCH v3 2/3] qapi: Do not generate empty enum
On Tue, Mar 21, 2023 at 03:31:56PM +0100, Philippe Mathieu-Daudé wrote: > On 16/3/23 15:57, Markus Armbruster wrote: > > Daniel P. Berrangé writes: > > > > > On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote: > > > > Philippe Mathieu-Daudé writes: > > > > > > > > > Per the C++ standard, empty enum are ill-formed. Do not generate > > > > The C standard. The C++ standard doesn't apply here :) > > I can't find how empty enums are considered by the C standard... The C standard doesn't really matter either. What we actually care about is whether GCC and CLang consider the empty enums to be permissible or not. or to put it another way... if it compiles, ship it :-) With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 2/3] qapi: Do not generate empty enum
On 16/3/23 15:57, Markus Armbruster wrote:
Daniel P. Berrangé writes:
But all of this falls apart with conditional members!
Example 1 (taken from qapi/block-core.json):
{ 'enum': 'BlockdevAioOptions',
'data': [ 'threads', 'native',
{ 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' } ] }
Generates now:
typedef enum BlockdevAioOptions {
BLOCKDEV_AIO_OPTIONS_THREADS,
BLOCKDEV_AIO_OPTIONS_NATIVE,
#if defined(CONFIG_LINUX_IO_URING)
BLOCKDEV_AIO_OPTIONS_IO_URING,
#endif /* defined(CONFIG_LINUX_IO_URING) */
BLOCKDEV_AIO_OPTIONS__MAX,
} BlockdevAioOptions;
BLOCKDEV_AIO_OPTIONS__MAX is 3 if defined(CONFIG_LINUX_IO_URING), else
2.
After the next patch:
typedef enum BlockdevAioOptions {
BLOCKDEV_AIO_OPTIONS_THREADS,
BLOCKDEV_AIO_OPTIONS_NATIVE,
#if defined(CONFIG_LINUX_IO_URING)
BLOCKDEV_AIO_OPTIONS_IO_URING,
#endif /* defined(CONFIG_LINUX_IO_URING) */
#define BLOCKDEV_AIO_OPTIONS__MAX 3
} BlockdevAioOptions;
Now it's always 3.
Example 2 (same with members reordered):
{ 'enum': 'BlockdevAioOptions',
'data': [ { 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' },
'threads', 'native' ] }
Same problem for __MAX, additional problem for __DUMMY:
typedef enum BlockdevAioOptions {
BLOCKDEV_AIO_OPTIONS__DUMMY = 0,
#if defined(CONFIG_LINUX_IO_URING)
BLOCKDEV_AIO_OPTIONS_IO_URING = 0,
#endif /* defined(CONFIG_LINUX_IO_URING) */
BLOCKDEV_AIO_OPTIONS_THREADS,
BLOCKDEV_AIO_OPTIONS_NATIVE,
#define BLOCKDEV_AIO_OPTIONS__MAX 3
} BlockdevAioOptions;
If CONFIG_LINUX_IO_URING is off, the enum starts at 1 instead of 0.
Arrays indexed by the enum start with a hole. Code using them is
probably not prepared for holes.
Can we meet half-way only generating the MAX definitions for
unconditional enums, keeping the conditional ones as is?
-- >8 --
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
@@ -88,16 +88,18 @@ def gen_enum(name: str,
members: List[QAPISchemaEnumMember],
prefix: Optional[str] = None) -> str:
assert members
-# append automatically generated _MAX value
-enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
-
ret = mcgen('''
typedef enum %(c_name)s {
''',
c_name=c_name(name))
-for memb in enum_members:
+has_cond = any(memb.ifcond.is_present() for memb in members)
+if has_cond:
+# append automatically generated _MAX value
+members += [QAPISchemaEnumMember('_MAX', None)]
+
+for memb in members:
ret += memb.ifcond.gen_if()
ret += mcgen('''
%(c_enum)s,
@@ -105,6 +107,13 @@ def gen_enum(name: str,
c_enum=c_enum_const(name, memb.name, prefix))
ret += memb.ifcond.gen_endif()
+if not has_cond:
+ret += mcgen('''
+#define %(c_name)s %(c_length)s
+''',
+ c_name=c_enum_const(name, '_MAX', prefix),
+ c_length=len(members))
+
ret += mcgen('''
} %(c_name)s;
''',
---
Re: [PATCH v3 2/3] qapi: Do not generate empty enum
On 16/3/23 15:57, Markus Armbruster wrote:
Daniel P. Berrangé writes:
On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:
Philippe Mathieu-Daudé writes:
Per the C++ standard, empty enum are ill-formed. Do not generate
The C standard. The C++ standard doesn't apply here :)
I can't find how empty enums are considered by the C standard...
But all of this falls apart with conditional members!
Example 1 (taken from qapi/block-core.json):
{ 'enum': 'BlockdevAioOptions',
'data': [ 'threads', 'native',
{ 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' } ] }
Generates now:
typedef enum BlockdevAioOptions {
BLOCKDEV_AIO_OPTIONS_THREADS,
BLOCKDEV_AIO_OPTIONS_NATIVE,
#if defined(CONFIG_LINUX_IO_URING)
BLOCKDEV_AIO_OPTIONS_IO_URING,
#endif /* defined(CONFIG_LINUX_IO_URING) */
BLOCKDEV_AIO_OPTIONS__MAX,
} BlockdevAioOptions;
BLOCKDEV_AIO_OPTIONS__MAX is 3 if defined(CONFIG_LINUX_IO_URING), else
2.
After the next patch:
typedef enum BlockdevAioOptions {
BLOCKDEV_AIO_OPTIONS_THREADS,
BLOCKDEV_AIO_OPTIONS_NATIVE,
#if defined(CONFIG_LINUX_IO_URING)
BLOCKDEV_AIO_OPTIONS_IO_URING,
#endif /* defined(CONFIG_LINUX_IO_URING) */
#define BLOCKDEV_AIO_OPTIONS__MAX 3
} BlockdevAioOptions;
Now it's always 3.
Good catch... Using:
-- >8 --
diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py
@@ -110,8 +110,11 @@ def gen_enum(name: str,
ret += mcgen('''
} %(c_name)s;
+_Static_assert(%(c_last_enum)s == %(c_length)s - 1, "%(c_name)s");
''',
- c_name=c_name(name))
+ c_name=c_name(name),
+ c_last_enum=c_enum_const(name, members[-1].name, prefix),
+ c_length=len(members))
ret += mcgen('''
---
I get:
./qapi/qapi-types-block-core.h:355:1: error: static_assert failed due to
requirement 'BLOCKDEV_AIO_OPTIONS_NATIVE == 3 - 1' "BlockdevAioOptions"
_Static_assert(BLOCKDEV_AIO_OPTIONS_IO_URING == 3 - 1,
"BlockdevAioOptions");
^ ~~
./qapi/qapi-types-block-core.h:430:1: error: static_assert failed due to
requirement 'BLOCKDEV_DRIVER_VVFAT == 47 - 1' "BlockdevDriver"
_Static_assert(BLOCKDEV_DRIVER_VVFAT == 47 - 1, "BlockdevDriver");
^ ~~~
./qapi/qapi-types-char.h:110:1: error: static_assert failed due to
requirement 'CHARDEV_BACKEND_KIND_MEMORY == 22 - 1' "ChardevBackendKind"
_Static_assert(CHARDEV_BACKEND_KIND_MEMORY == 22 - 1, "ChardevBackendKind");
^ ~
...
Re: [PATCH v3 2/3] qapi: Do not generate empty enum
Daniel P. Berrangé writes:
> On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé writes:
>>
>> > Per the C++ standard, empty enum are ill-formed. Do not generate
The C standard. The C++ standard doesn't apply here :)
>> > them in order to avoid:
>> >
>> > In file included from qga/qga-qapi-emit-events.c:14:
>> > qga/qga-qapi-emit-events.h:20:1: error: empty enum is invalid
>> > 20 | } qga_QAPIEvent;
>> > | ^
>> >
>> > Reported-by: Markus Armbruster
>> > Signed-off-by: Philippe Mathieu-Daudé
>>
>> Two failures in "make check-qapi-schema" (which is run by "make check"):
>>
>> 1. Positive test case qapi-schema-test
>>
>> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/qapi-schema-test.out
>> +++
>> @@ -19,7 +19,6 @@
>> member enum2: EnumOne optional=True
>> member enum3: EnumOne optional=False
>> member enum4: EnumOne optional=True
>> -enum MyEnum
>> object Empty1
>> object Empty2
>> base Empty1
>>
>>You forgot to update expected test output. No big deal.
>>
>> 2. Negative test case union-empty
>>
>> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/union-empty.err
>> +++
>> @@ -1,2 +1,2 @@
>> -union-empty.json: In union 'Union':
>> -union-empty.json:4: union has no branches
>> +union-empty.json: In struct 'Base':
>> +union-empty.json:3: member 'type' uses unknown type 'Empty'
>> stderr:
>> qapi-schema-test FAIL
>> union-empty FAIL
>>
>>The error message regresses.
>>
>>I can see two ways to fix this:
>>
>>(A) You can't just drop empty enumeration types on the floor. To not
>>generate code for them, you need to skip them wherever we
>>generate code for enumeration types.
>>
>>(B) Outlaw empty enumeration types.
>>
>> I recommend to give (B) a try, it's likely simpler.
>
> Possible trap-door with (B), if we have any enums where *every*
> member is conditionalized on a CONFIG_XXX rule, there might be
> certain build scenarios where an enum suddenly becomes empty.
True. Scratch the idea.
Trap-door also applies to (A): we can still end up with empty enums.
(C) Always emit a dummy member. This is actually what we do now:
typedef enum OnOffAuto {
ON_OFF_AUTO_AUTO = 1,
ON_OFF_AUTO_ON = 2,
ON_OFF_AUTO_OFF = 3,
ON_OFF_AUTO__MAX, <--- the dummy
} OnOffAuto;
But the next patch changes it to
typedef enum OnOffAuto {
ON_OFF_AUTO_AUTO,
ON_OFF_AUTO_ON,
ON_OFF_AUTO_OFF,
#define ON_OFF_AUTO__MAX 3
} OnOffAuto;
Two problems, actually.
One, we lose the dummy. We could add one back like
typedef enum OnOffAuto {
ON_OFF_AUTO__DUMMY = 0,
ON_OFF_AUTO_AUTO = 0,
ON_OFF_AUTO_ON,
ON_OFF_AUTO_OFF,
#define ON_OFF_AUTO__MAX 3
} OnOffAuto;
But all of this falls apart with conditional members!
Example 1 (taken from qapi/block-core.json):
{ 'enum': 'BlockdevAioOptions',
'data': [ 'threads', 'native',
{ 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' } ] }
Generates now:
typedef enum BlockdevAioOptions {
BLOCKDEV_AIO_OPTIONS_THREADS,
BLOCKDEV_AIO_OPTIONS_NATIVE,
#if defined(CONFIG_LINUX_IO_URING)
BLOCKDEV_AIO_OPTIONS_IO_URING,
#endif /* defined(CONFIG_LINUX_IO_URING) */
BLOCKDEV_AIO_OPTIONS__MAX,
} BlockdevAioOptions;
BLOCKDEV_AIO_OPTIONS__MAX is 3 if defined(CONFIG_LINUX_IO_URING), else
2.
After the next patch:
typedef enum BlockdevAioOptions {
BLOCKDEV_AIO_OPTIONS_THREADS,
BLOCKDEV_AIO_OPTIONS_NATIVE,
#if defined(CONFIG_LINUX_IO_URING)
BLOCKDEV_AIO_OPTIONS_IO_URING,
#endif /* defined(CONFIG_LINUX_IO_URING) */
#define BLOCKDEV_AIO_OPTIONS__MAX 3
} BlockdevAioOptions;
Now it's always 3.
Example 2 (same with members reordered):
{ 'enum': 'BlockdevAioOptions',
'data': [ { 'name': 'io_uring', 'if': 'CONFIG_LINUX_IO_URING' },
'threads', 'native' ] }
Same problem for __MAX, additional problem for __DUMMY:
typedef enum BlockdevAioOptions {
BLOCKDEV_AIO_OPTIONS__DUMMY = 0,
#if defined(CONFIG_LINUX_IO_URING)
BLOCKDEV_AIO_OPTIONS_IO_URING = 0,
#endif /* defined(CONFIG_LINUX_IO_URING) */
BLOCKDEV_AIO_OPTIONS_THREADS,
BLOCKDEV_AIO_OPTIONS_NATIVE,
#define BLOCKDEV_AIO_OPTIONS__MAX 3
} BlockdevAioOptions;
If CONFIG_LINUX_IO_URING is off, the enum starts at 1 instead of 0.
Arrays indexed by the enum start with a hole. Code using them is
probably not prepared for holes.
*Sigh*
Re: [PATCH v3 2/3] qapi: Do not generate empty enum
On Thu, Mar 16, 2023 at 03:39:59PM +0100, Juan Quintela wrote:
> Daniel P. Berrangé wrote:
> > On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote:
> >> Philippe Mathieu-Daudé writes:
> >>
> >> > Per the C++ standard, empty enum are ill-formed. Do not generate
> >> > them in order to avoid:
> >> >
> >> > In file included from qga/qga-qapi-emit-events.c:14:
> >> > qga/qga-qapi-emit-events.h:20:1: error: empty enum is invalid
> >> > 20 | } qga_QAPIEvent;
> >> > | ^
> >> >
> >> > Reported-by: Markus Armbruster
> >> > Signed-off-by: Philippe Mathieu-Daudé
> >>
> >> Two failures in "make check-qapi-schema" (which is run by "make check"):
> >>
> >> 1. Positive test case qapi-schema-test
> >>
> >> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/qapi-schema-test.out
> >> +++
> >> @@ -19,7 +19,6 @@
> >> member enum2: EnumOne optional=True
> >> member enum3: EnumOne optional=False
> >> member enum4: EnumOne optional=True
> >> -enum MyEnum
> >> object Empty1
> >> object Empty2
> >> base Empty1
> >>
> >>You forgot to update expected test output. No big deal.
> >>
> >> 2. Negative test case union-empty
> >>
> >> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/union-empty.err
> >> +++
> >> @@ -1,2 +1,2 @@
> >> -union-empty.json: In union 'Union':
> >> -union-empty.json:4: union has no branches
> >> +union-empty.json: In struct 'Base':
> >> +union-empty.json:3: member 'type' uses unknown type 'Empty'
> >> stderr:
> >> qapi-schema-test FAIL
> >> union-empty FAIL
> >>
> >>The error message regresses.
> >>
> >>I can see two ways to fix this:
> >>
> >>(A) You can't just drop empty enumeration types on the floor. To not
> >>generate code for them, you need to skip them wherever we
> >>generate code for enumeration types.
> >>
> >>(B) Outlaw empty enumeration types.
> >>
> >> I recommend to give (B) a try, it's likely simpler.
> >
> > Possible trap-door with (B), if we have any enums where *every*
> > member is conditionalized on a CONFIG_XXX rule, there might be
> > certain build scenarios where an enum suddenly becomes empty.
>
> Do we have an example for this?
> Because it looks really weird. I would expect that the "container" unit
> of that enumeration is #ifdef out of compilation somehow.
I'm not sure if such an example physically exists. I know the audio
code gets close, with all but 2 options conditional:
{ 'enum': 'AudiodevDriver',
'data': [ 'none',
{ 'name': 'alsa', 'if': 'CONFIG_AUDIO_ALSA' },
{ 'name': 'coreaudio', 'if': 'CONFIG_AUDIO_COREAUDIO' },
{ 'name': 'dbus', 'if': 'CONFIG_DBUS_DISPLAY' },
{ 'name': 'dsound', 'if': 'CONFIG_AUDIO_DSOUND' },
{ 'name': 'jack', 'if': 'CONFIG_AUDIO_JACK' },
{ 'name': 'oss', 'if': 'CONFIG_AUDIO_OSS' },
{ 'name': 'pa', 'if': 'CONFIG_AUDIO_PA' },
{ 'name': 'sdl', 'if': 'CONFIG_AUDIO_SDL' },
{ 'name': 'sndio', 'if': 'CONFIG_AUDIO_SNDIO' },
{ 'name': 'spice', 'if': 'CONFIG_SPICE' },
'wav' ] }
Just wanted to warn that we shouldn't assume empty enums can't
exist, because it would be quite easy to add 2 extra conditionals
to this audio example, and the enum wouldn't appear empty at a
glance, but none the less could be empty in some compile scenarios
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 2/3] qapi: Do not generate empty enum
Daniel P. Berrangé wrote: > On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote: >> Philippe Mathieu-Daudé writes: >> >> > Per the C++ standard, empty enum are ill-formed. Do not generate >> > them in order to avoid: >> > >> > In file included from qga/qga-qapi-emit-events.c:14: >> > qga/qga-qapi-emit-events.h:20:1: error: empty enum is invalid >> > 20 | } qga_QAPIEvent; >> > | ^ >> > >> > Reported-by: Markus Armbruster >> > Signed-off-by: Philippe Mathieu-Daudé >> >> Two failures in "make check-qapi-schema" (which is run by "make check"): >> >> 1. Positive test case qapi-schema-test >> >> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/qapi-schema-test.out >> +++ >> @@ -19,7 +19,6 @@ >> member enum2: EnumOne optional=True >> member enum3: EnumOne optional=False >> member enum4: EnumOne optional=True >> -enum MyEnum >> object Empty1 >> object Empty2 >> base Empty1 >> >>You forgot to update expected test output. No big deal. >> >> 2. Negative test case union-empty >> >> --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/union-empty.err >> +++ >> @@ -1,2 +1,2 @@ >> -union-empty.json: In union 'Union': >> -union-empty.json:4: union has no branches >> +union-empty.json: In struct 'Base': >> +union-empty.json:3: member 'type' uses unknown type 'Empty' >> stderr: >> qapi-schema-test FAIL >> union-empty FAIL >> >>The error message regresses. >> >>I can see two ways to fix this: >> >>(A) You can't just drop empty enumeration types on the floor. To not >>generate code for them, you need to skip them wherever we >>generate code for enumeration types. >> >>(B) Outlaw empty enumeration types. >> >> I recommend to give (B) a try, it's likely simpler. > > Possible trap-door with (B), if we have any enums where *every* > member is conditionalized on a CONFIG_XXX rule, there might be > certain build scenarios where an enum suddenly becomes empty. Do we have an example for this? Because it looks really weird. I would expect that the "container" unit of that enumeration is #ifdef out of compilation somehow. Later, Juan.
Re: [PATCH v3 2/3] qapi: Do not generate empty enum
On Thu, Mar 16, 2023 at 01:31:04PM +0100, Markus Armbruster wrote: > Philippe Mathieu-Daudé writes: > > > Per the C++ standard, empty enum are ill-formed. Do not generate > > them in order to avoid: > > > > In file included from qga/qga-qapi-emit-events.c:14: > > qga/qga-qapi-emit-events.h:20:1: error: empty enum is invalid > > 20 | } qga_QAPIEvent; > > | ^ > > > > Reported-by: Markus Armbruster > > Signed-off-by: Philippe Mathieu-Daudé > > Two failures in "make check-qapi-schema" (which is run by "make check"): > > 1. Positive test case qapi-schema-test > > --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/qapi-schema-test.out > +++ > @@ -19,7 +19,6 @@ > member enum2: EnumOne optional=True > member enum3: EnumOne optional=False > member enum4: EnumOne optional=True > -enum MyEnum > object Empty1 > object Empty2 > base Empty1 > >You forgot to update expected test output. No big deal. > > 2. Negative test case union-empty > > --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/union-empty.err > +++ > @@ -1,2 +1,2 @@ > -union-empty.json: In union 'Union': > -union-empty.json:4: union has no branches > +union-empty.json: In struct 'Base': > +union-empty.json:3: member 'type' uses unknown type 'Empty' > stderr: > qapi-schema-test FAIL > union-empty FAIL > >The error message regresses. > >I can see two ways to fix this: > >(A) You can't just drop empty enumeration types on the floor. To not >generate code for them, you need to skip them wherever we >generate code for enumeration types. > >(B) Outlaw empty enumeration types. > > I recommend to give (B) a try, it's likely simpler. Possible trap-door with (B), if we have any enums where *every* member is conditionalized on a CONFIG_XXX rule, there might be certain build scenarios where an enum suddenly becomes empty. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v3 2/3] qapi: Do not generate empty enum
Philippe Mathieu-Daudé writes: > Per the C++ standard, empty enum are ill-formed. Do not generate > them in order to avoid: > > In file included from qga/qga-qapi-emit-events.c:14: > qga/qga-qapi-emit-events.h:20:1: error: empty enum is invalid > 20 | } qga_QAPIEvent; > | ^ > > Reported-by: Markus Armbruster > Signed-off-by: Philippe Mathieu-Daudé Two failures in "make check-qapi-schema" (which is run by "make check"): 1. Positive test case qapi-schema-test --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/qapi-schema-test.out +++ @@ -19,7 +19,6 @@ member enum2: EnumOne optional=True member enum3: EnumOne optional=False member enum4: EnumOne optional=True -enum MyEnum object Empty1 object Empty2 base Empty1 You forgot to update expected test output. No big deal. 2. Negative test case union-empty --- /work/armbru/qemu/bld-x86/../tests/qapi-schema/union-empty.err +++ @@ -1,2 +1,2 @@ -union-empty.json: In union 'Union': -union-empty.json:4: union has no branches +union-empty.json: In struct 'Base': +union-empty.json:3: member 'type' uses unknown type 'Empty' stderr: qapi-schema-test FAIL union-empty FAIL The error message regresses. I can see two ways to fix this: (A) You can't just drop empty enumeration types on the floor. To not generate code for them, you need to skip them wherever we generate code for enumeration types. (B) Outlaw empty enumeration types. I recommend to give (B) a try, it's likely simpler.
Re: [PATCH v3 2/3] qapi: Do not generate empty enum
On 3/15/23 04:28, Philippe Mathieu-Daudé wrote: Per the C++ standard, empty enum are ill-formed. Do not generate them in order to avoid: In file included from qga/qga-qapi-emit-events.c:14: qga/qga-qapi-emit-events.h:20:1: error: empty enum is invalid 20 | } qga_QAPIEvent; | ^ Reported-by: Markus Armbruster Signed-off-by: Philippe Mathieu-Daudé --- docs/devel/qapi-code-gen.rst | 6 +++--- scripts/qapi/events.py | 2 ++ scripts/qapi/schema.py | 5 - scripts/qapi/types.py| 2 ++ 4 files changed, 11 insertions(+), 4 deletions(-) Reviewed-by: Richard Henderson r~
