Re: [Qemu-devel] [PATCH V8 00/10] qapi script: support enum as discriminator and better enum name

[Luiz Capitulino]

On Tue, 04 Mar 2014 14:03:34 +0100
Markus Armbruster <address@hidden> wrote:

> Wenchao Xia <address@hidden> writes:
> 
> > This series address two issues:
> >
> > 1. support using enum as discriminator in union.
> > For example, if we have following define in qapi schema:
> > { 'enum': 'EnumOne',
> >   'data': [ 'value1', 'value2', 'value3' ] }
> >
> > { 'type': 'UserDefBase0',
> >   'data': { 'base-string0': 'str', 'base-enum0': 'EnumOne' } }
> >
> > Before this series, discriminator in union must be a string, and a
> > hidden enum type as discriminator is generated. After this series,
> > qapi schema can directly use predefined enum type:
> > { 'union': 'UserDefEnumDiscriminatorUnion',
> >   'base': 'UserDefBase0',
> >   'discriminator' : 'base-enum0',
> >   'data': { 'value1' : 'UserDefA',
> >             'value2' : 'UserDefInherit',
> >             'value3' : 'UserDefB' } }
> >
> > The benefit is that every thing is defined explicitly in schema file,
> > the discriminator enum type can be used in other API define in schema,
> > and a compile time check will be put to verify the correctness according
> > to enum define. Currently BlockdevOptions used discriminator which can
> > be converted, in the future other union can also use enum discriminator.
> >
> > The implement is done by:
> > 1.1 remember the enum defines by qapi scripts.(patch 1)
> > 1.2 use the remembered enum define to check correctness at compile
> > time.(patch 3), more strict check(patch 2)
> > 1.3 use the same enum name generation rule to avoid C code mismatch,
> > esp for "case [ENUM_VALUE]" in qapi-visit.c.(patch 4,5)
> > 1.4 switch the code path, when pre-defined enum type is used as 
> > discriminator,
> > don't generate a hidden enum type, use the enum type instead, add
> > docs/qapi-code-gen.txt.(Patch 6)
> > 1.5 test case shows how it looks like.(Patch 7)
> > 1.6 convert BlockdevOptions. (Patch 8)
> >
> > 2. Better enum name generation
> > Before this patch, AIOContext->A_I_O_CONTEXT, after this patch,
> > AIOContet->AIO_CONTEXT. Since previous patch has foldered enum
> > name generation codes into one function, it is done easily by modifying
> > it.(Patch 9)
> [...]
> > v8:
> >   The series is ontop of Markus's tree and rebased on upstream:
> >   Address Markus's comments:
> >   1/10: better commit title, simplify is_enum().
> >   2/10: test case squashed into this patch.
> >   3/10: no change, column computation is not touched.
> >   4/10: simplify commit title and message, refine the semantic check
> > logic as comments in v7, check in discriminator_find_enum_define() is moved
> > out so that the function can be used without error info, squash related test
> > into this patch, re-orgnize 'expr_elem' with separate 'info' member,
> > QAPIExprError now takes only info to work, better error message, remove
> > check of whether all enum values are covered by branch, use expr['key']
> > instead of expr.get('key') when possible.
> >   6/10: remove useless comments in generate_enum_full_value(), make line
> > shorter by change variable name.
> >   7/10: building 'expr_elem' for discriminator_find_enum_define() is 
> > removed,
> > make line shorter in qapi-visit.py, add a test case that enum is used before
> > define.
> >   8/10: rebased on uptream by adding 'quorum' driver type.
> >   9/10: better doc and error message, add a test case for string 
> > discriminator.
> > use string concatenation rather than line continuation for string in C code.
> 
> Applies cleanly on Luiz's queue/qmp branch.
> 
> I got a few questions on 04/10, but nothing major.  Perhaps you'd like
> to respin to address the nits picked by Eric and me.  If not, we can ask
> Luiz to clean up commit messages on commit as per Eric's review.

Not sure if that's what you meant, but Wenchao could respin only 04/10.

> 
> Reviewed-by: Markus Armbruster <address@hidden>
>