Re: [PATCH 25/27] include/qapi: add g_autoptr support for qobject types

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Wed, Mar 16, 2022 at 4:31 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Add small inline wrappers for qobject_unref() calls, which is a macro.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  include/qapi/qmp/qbool.h   | 6 ++++++
>> >  include/qapi/qmp/qdict.h   | 6 ++++++
>> >  include/qapi/qmp/qlist.h   | 8 +++++++-
>> >  include/qapi/qmp/qnull.h   | 6 ++++++
>> >  include/qapi/qmp/qnum.h    | 6 ++++++
>> >  include/qapi/qmp/qstring.h | 6 ++++++
>> >  6 files changed, 37 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/include/qapi/qmp/qbool.h b/include/qapi/qmp/qbool.h
>> > index 2f888d10573f..52b1c5c15280 100644
>> > --- a/include/qapi/qmp/qbool.h
>> > +++ b/include/qapi/qmp/qbool.h
>> > @@ -21,6 +21,12 @@ struct QBool {
>> >      bool value;
>> >  };
>> >
>> > +static inline void qbool_unref(QBool *q) {
>> > +    qobject_unref(q);
>> > +}
>> > +
>> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(QBool, qbool_unref)
>> > +
>>
>> You need the wrapper function around the wrapper macro qobject_unref(),
>> because
>>
>>    G_DEFINE_AUTOPTR_CLEANUP_FUNC(QBool, qobject_unref_impl)
>>
>> dies with "passing argument 1 of ‘qobject_unref_impl’ from incompatible
>> pointer type [-Wincompatible-pointer-types]".  Okay.
>
> Yeah, unfortunately. There might be other tricks possible, but they
> would likely be less obvious.
>
>>
>> Style nitpick: a function's opening brace goes on its own line:
>>
>>    static inline void qbool_unref(QBool *q)
>>    {
>>        qobject_unref(q);
>>    }
>>
>
> right
>
>> Moreover, I prefer to put code in headers only when there's a real need.
>> I don't see one here.  Most existing uses of
>
> I agree, I generally don't like inline. However, simple one-liner
> wrapper kinda fit. I was even tempted to use extra _ at the end of the
> function to prevent usage outside of the macro, but decided that
> wouldn't be much better.
>
> Btw, what's' your rule for using "static inline" in headers then :) ?

Demonstrated performance improvement would be compelling.

Occasionally, there's no good .c home for the function, and putting it
in the header is overall less bad than creating a .c for it.

>> G_DEFINE_AUTOPTR_CLEANUP_FUNC() use a plain extern function.

[...]