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

[Marc-André Lureau]

On Mon, Dec 10, 2018 at 2:11 PM Markus Armbruster <address@hidden> wrote:
>
> Markus Armbruster <address@hidden> writes:
>
> > Marc-André Lureau <address@hidden> writes:
> >
> >> Hi
> >> On Thu, Dec 6, 2018 at 9:42 PM Markus Armbruster <address@hidden> wrote:
> >>>
> >>> Marc-André Lureau <address@hidden> 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