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

[Markus Armbruster]

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.

Reviewed-by: Markus Armbruster <address@hidden>