Re: [Qemu-devel] [PATCH v6 21/27] qapi: add #if conditions to generated code members

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
> On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> 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.

[...]