Re: [Qemu-devel] [PATCH v5 06/14] qapi-event: Slightly shrink generated code

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Slightly rearrange the code in gen_event_send() for less generated
> output, by initializing 'emit' sooner, deferring an assertion
> to qdict_put_obj, and dropping a now-unused 'obj' local variable.
>
> While at it, document a FIXME related to the potential for a
> compiler naming collision - if the user ever creates a QAPI event
> whose name matches 'errp' or one of our local variables (like
> 'emit'), we'll have to revisit how we generate functions to
> avoid the problem.
>
> |@@ -25,16 +25,13 @@ void qapi_event_send_acpi_device_ost(ACP
> | {
> |     QDict *qmp;
> |     Error *err = NULL;
> |-    QMPEventFuncEmit emit;
> |+    QMPEventFuncEmit emit = qmp_event_get_func_emit();
> |     QmpOutputVisitor *qov;
> |     Visitor *v;
> |-    QObject *obj;
> |
> |-    emit = qmp_event_get_func_emit();
> |     if (!emit) {
> |         return;
> |     }
> |-
> |     qmp = qmp_event_build_dict("ACPI_DEVICE_OST");
> |
> |     qov = qmp_output_visitor_new();
> |@@ -53,11 +50,7 @@ out_obj:
> |     if (err) {
> |         goto out;
> |     }
> |-
> |-    obj = qmp_output_get_qobject(qov);
> |-    g_assert(obj);

We're not "deferring an assertion to qdict_put_obj()", we're dropping a
dead one: qmp_output_get_qobject() never returns null.

I figure the assertion dates back to the time when it still did.  Back
then, getting null here meant we screwed up.

I just searched the code for similarly dead assertions.  Found one in
qapi_clone_InputEvent(), and serveral more in test-qmp-output-visitor.c.

There's also an error return in qapi_copy_SocketAddress().  Useless?
Should check for qnull instead?

> |-
> |-    qdict_put_obj(qmp, "data", obj);
> |+    qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov));
> |     emit(QAPI_EVENT_ACPI_DEVICE_OST, qmp, &err);
> |
> | out:
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v5: new patch
> ---
>  scripts/qapi-event.py | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/scripts/qapi-event.py b/scripts/qapi-event.py
> index c03cb78..02c9556 100644
> --- a/scripts/qapi-event.py
> +++ b/scripts/qapi-event.py
> @@ -29,13 +29,19 @@ def gen_event_send_decl(name, arg_type):
>
>
>  def gen_event_send(name, arg_type):
> +    # FIXME: Our declaration of local variables (and of 'errp' in the
> +    # parameter list) can collide with exploded members of the event's
> +    # data type passed in as parameters.  If this collision ever hits in
> +    # practice, we can rename our local variables with a leading _ prefix,
> +    # or split the code into a wrapper function that creates a boxed
> +    # 'param' object then calls another to do the real work.
>      ret = mcgen('''
>
>  %(proto)s
>  {
>      QDict *qmp;
>      Error *err = NULL;
> -    QMPEventFuncEmit emit;
> +    QMPEventFuncEmit emit = qmp_event_get_func_emit();
>  ''',
>                  proto=gen_event_send_proto(name, arg_type))
>
> @@ -43,16 +49,13 @@ def gen_event_send(name, arg_type):
>          ret += mcgen('''
>      QmpOutputVisitor *qov;
>      Visitor *v;
> -    QObject *obj;
> -

Please keep the blank line here...

>  ''')
>
>      ret += mcgen('''
> -    emit = qmp_event_get_func_emit();
> +

... instead of adding it here.

>      if (!emit) {
>          return;
>      }
> -

Let's keep this one.

>      qmp = qmp_event_build_dict("%(name)s");
>
>  ''',
> @@ -76,11 +79,7 @@ out_obj:
>      if (err) {
>          goto out;
>      }
> -
> -    obj = qmp_output_get_qobject(qov);
> -    g_assert(obj);
> -
> -    qdict_put_obj(qmp, "data", obj);
> +    qdict_put_obj(qmp, "data", qmp_output_get_qobject(qov));
>  ''')
>
>      ret += mcgen('''

Small improvements are welcome, too :)