Re: [Qemu-devel] [PATCH V2 0/8] qapi script: support enum as discriminator and better enum name

[Wenchao Xia]

于 2013/11/28 22:24, Luiz Capitulino 写道:
> On Thu, 28 Nov 2013 14:19:48 +0800
> Wenchao Xia <address@hidden> wrote:
>
> > 于 2013/11/26 0:47, Luiz Capitulino 写道:
> > > On Wed, 13 Nov 2013 06:25:00 +0800
> > > Wenchao Xia <address@hidden> wrote:
> > >
> > > > This series is respined from RFC series at:
> > > > http://lists.nongnu.org/archive/html/qemu-devel/2013-11/msg00363.html
> > > >
> > > > Patch 1-6 add support for enum as discriminator.
> > > > Patch 7 improve enum name generation, now AIOContext->AIO_CONTEXT, X86CPU->
> > > > X86_CPU.
> > > > Patch 8 are the test cases.
> > >
> > > Can you please clarify what is the problem this series is trying to
> > > solve, how it does it and provide before/after type of examples?
> > >
> > > That's what I'd expect from an intro email, but this one has only a
> > > reference to an RFC series that has no better info, and some crypt
> > > changelog with magic numbers :(
> > >
> > > Besides, this doesn't apply anymore...
> >     Let me have an introduction:
> >
> > 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' } }
>
> OK, now tell me who will benefit from this. You don't have to answer
> right now, you can resend your series with this information. Note that
> I'm not asking to be convinced, I'm asking to have enough info so that
> I can understand what this is about when I get to review this (and this
> is just what an intro email is about).

  OK, I will send v3 with better doc.

> > 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)
> > 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 when pre-defined enum type is used as discriminator, don't generate
> > a hidden enum type, switch the code path to support enum as
> > discriminator.
> > 1.5 test case shows how it looks like.(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 7)
> >
> > > > Changes from RFC:
> > > >     Mainly address Eric's comments: fix typo, add patch 2 to allow partly 
> > > > mapping
> > > > enum value in union, add related test case, remove direct inherit support 
> > > > "_base"
> > > > and related test case.
> > > >
> > > > v2:
> > > >     General:
> > > >     3: use Raise exception instead of sys.error.write in qapi.py.
> > > >     Address Eric's comments:
> > > >     2,3: more check for enum value at compile time.
> > > >     8: correspond test case change.
> > > >
> > > > Wenchao Xia (8):
> > > >     1 qapi script: remember enum values
> > > >     2 qapi script: add check for duplicated key
> > > >     3 qapi script: check correctness of discriminator values in union
> > > >     4 qapi script: code move for generate_enum_name()
> > > >     5 qapi script: use same function to generate enum string
> > > >     6 qapi script: not generate hidden enum type for pre-defined enum 
> > > > discriminator
> > > >     7 qapi script: do not add "_" for every capitalized char in enum
> > > >     8 tests: add cases for inherited struct and union with discriminator
> > > >
> > > >    include/qapi/qmp/qerror.h               |    2 +-
> > > >    scripts/qapi-types.py                   |   34 ++++----
> > > >    scripts/qapi-visit.py                   |   55 +++++++++--
> > > >    scripts/qapi.py                         |   84 ++++++++++++++++-
> > > >    target-i386/cpu.c                       |    2 +-
> > > >    tests/qapi-schema/comments.out          |    2 +-
> > > >    tests/qapi-schema/qapi-schema-test.json |   27 ++++++
> > > >    tests/qapi-schema/qapi-schema-test.out  |   15 +++-
> > > >    tests/test-qmp-input-visitor.c          |  120 +++++++++++++++++++++++++
> > > >    tests/test-qmp-output-visitor.c         |  149 
> > > > +++++++++++++++++++++++++++++++
> > > >    10 files changed, 454 insertions(+), 36 deletions(-)