Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> Hi
>
> On Tue, Dec 18, 2018 at 10:27 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> This reverts commit 7bd263490590ee6fcf34ecb6203437e22f6e5a9c.
>>
>> The commit applied the events' conditions to the members of enum
>> QAPIEvent. Awkward, because it renders QAPIEvent unusable in
>> target-independent code as soon as we make an event target-dependent.
>> Reverting this has the following effects:
>>
>> * ui/vnc.c can remain target independent.
>
> Was it ever moved? I don't recall
It's currently target-independent, and we want it to remain that way.
You keep it that way by splitting target-dependent target_QAPIEvent off
QAPIEvent.
I keep it that way by not making any enum members target-dependent.
>> * monitor_qapi_event_conf[] doesn't have to muck around with #ifdef.
>
> I suggested a way to get rid of monitor_qapi_event_conf[] in the patch
> message, by having the rate stored in the schema, which could actually
> be useful (for doc, introspection etc).
Introspection and generated documentation improvements might still make
that worthwhile. For the former, we'd first want an actual user,
though.
>> * query-events again doesn't reflect conditionals. I'm going to
>> deprecate it in favor of query-qmp-schema.
>
> I guess that's not that important.
query-qmp-schema has reflected conditionals since we introduced them in
v3.0. query-events has not. Your commit fixes it for 4.0. Fixes are
good, but when an interface is known to be deficient in some versions,
while a strictly more powerful buddy is fine in all versions, the
obvious thing to do is to stay away from the deficient one regardless of
version.
I think we should deprecate query-events even if we decide to fix it
now.
I'll work this into the next commit's commit message.
> I have a slight preference for not declaring enums when the related
> option is disabled.
Me too, but I like keeping things simple even more.
Possibly even simpler: dispense with the enumeration type, add suitable
#define to each qapi-event-MODULE.h. We can have #ifdef there. What do
you think?
> But people don't like having too much #ifdef code, which is understandable.
In this case, it's just one big #if in monitor.c. Tolerable, I think.
If we replace QAPIEvent by #define, the #if shrinks to just #ifdef
QAPI_EVENT_RTC_CHANGE.
Note that monitor.c is already target-dependent. It should really be
split up, though. The #if would keep the QMP part target-dependent,
unless we split it up even more. Hmm.
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> # Conflicts:
>> # scripts/qapi/events.py
>> ---
>> scripts/qapi/events.py | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/qapi/events.py b/scripts/qapi/events.py
>> index e988e43941..c944ba90b8 100644
>> --- a/scripts/qapi/events.py
>> +++ b/scripts/qapi/events.py
>> @@ -194,7 +194,9 @@ void %(event_emit)s(%(event_enum)s event, QDict *qdict);
>> self._genc.add(gen_event_send(name, arg_type, boxed,
>> self._event_enum_name,
>> self._event_emit_name))
>> - self._event_enum_members.append(QAPISchemaMember(name, ifcond))
>> + # Note: we generate the enum member regardless of @ifcond, to
>> + # keep the enumeration usable in target-independent code.
>> + self._event_enum_members.append(QAPISchemaMember(name))
>>
>>
>> def gen_events(schema, output_dir, prefix):
>> --
>> 2.17.2
>>
>>