Re: [Qemu-devel] [PATCH RFC 14/14] qapi: Add #if conditions to commands, events, types, visitors

[Markus Armbruster <armbru@redhat.com>]

Marc-Andre Lureau <mlureau@redhat.com> writes:

> Hi
>
> On Mon, Feb 12, 2018 at 8:22 AM, Markus Armbruster <armbru@redhat.com> wrote:
>> Example change to generated code:
>>
>>     diff -rup test-qapi-events.h.old test-qapi-events.h
>>     --- test-qapi-events.h.old  2018-02-12 07:02:45.672737544 +0100
>>     +++ test-qapi-events.h      2018-02-12 07:03:01.128517669 +0100
>>     @@ -30,8 +30,10 @@ void qapi_event_send_event_e(UserDefZero
>>      void qapi_event_send_event_f(UserDefAlternate *arg, Error **errp);
>>
>>      void qapi_event_send___org_qemu_x_event(__org_qemu_x_Enum __org_qemu_x_member1, const char *__org_qemu_x_member2, bool has_q_wchar_t, int64_t q_wchar_t, Error **errp);
>>     +#if defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT)
>>
>>      void qapi_event_send_testifevent(TestIfStruct *foo, Error **errp);
>>     +#endif /* defined(TEST_IF_EVT) && defined(TEST_IF_STRUCT) */
>>
>>      typedef enum test_QAPIEvent {
>>          TEST_QAPI_EVENT_EVENT_A = 0,
>>
>> TODO nice blank lines before #if and after #endif
>> FIXME unclean access of protected members in commands.py
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> I reviewed the RFC series, and your approach to visit ifcond instead
> of having them as attribute argument for the visitor (something you
> suggested me initially iirc).

I think I tossed out the idea, no more.

> I think the approach is interesting but that single patch shows the
> complexity involved. The decorator approach still looks cleaner and
> simpler to me.

De gustibus...

For what it's worth, I disliked the decorator magic enough to write this
series.

>                Furthermore, I don't fancy much having to redo and tune
> the generation *again* to fix the inden, extra-spaces etc that were
> fixed after several revisions (it takes hours to get there, it's
> boring). Can't we go first with my approach and then look at replacing
> it? Can't one add a "FIXME: replace the decorator with something less
> magic" at ifcond_decorator definition for now? Is this code so
> critical that it has to be the way you want in the first place? The
> approach to take it first and improve it worked very well for
> qapi2texi, it just took a few more days for you (and reviewers) to
> improve it. I'd suggest we work that way instead of having me rewrite
> and rewrite until you are happy (which is something I can't do right
> without many painful iterations for you and me).

This is up to the backup QAPI maintainer now :)