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

[Marc-André Lureau]

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 :) ?

> G_DEFINE_AUTOPTR_CLEANUP_FUNC() use a plain extern function.
>
> >  QBool *qbool_from_bool(bool value);
> >  bool qbool_get_bool(const QBool *qb);
> >
> > diff --git a/include/qapi/qmp/qdict.h b/include/qapi/qmp/qdict.h
> > index d5b5430e21a9..9f0a6a6708b5 100644
> > --- a/include/qapi/qmp/qdict.h
> > +++ b/include/qapi/qmp/qdict.h
> > @@ -30,6 +30,12 @@ struct QDict {
> >      QLIST_HEAD(,QDictEntry) table[QDICT_BUCKET_MAX];
> >  };
> >
> > +static inline void qdict_unref(QDict *q) {
> > +    qobject_unref(q);
> > +}
> > +
> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(QDict, qdict_unref)
> > +
> >  /* Object API */
> >  QDict *qdict_new(void);
> >  const char *qdict_entry_key(const QDictEntry *entry);
> > diff --git a/include/qapi/qmp/qlist.h b/include/qapi/qmp/qlist.h
> > index 06e98ad5f498..06c267dfb898 100644
> > --- a/include/qapi/qmp/qlist.h
> > +++ b/include/qapi/qmp/qlist.h
> > @@ -26,7 +26,13 @@ struct QList {
> >      QTAILQ_HEAD(,QListEntry) head;
> >  };
> >
> > -#define qlist_append(qlist, obj) \
> > +static inline void qlist_unref(QList *q) {
> > +    qobject_unref(q);
> > +}
> > +
> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(QList, qlist_unref)
> > +
> > +#define qlist_append(qlist, obj)                \
>
> The whitespace change looks accidental.
>
> >          qlist_append_obj(qlist, QOBJECT(obj))
> >
> >  void qlist_append_bool(QList *qlist, bool value);
> > diff --git a/include/qapi/qmp/qnull.h b/include/qapi/qmp/qnull.h
> > index e84ecceedbcb..8c45e08b1c47 100644
> > --- a/include/qapi/qmp/qnull.h
> > +++ b/include/qapi/qmp/qnull.h
> > @@ -26,4 +26,10 @@ static inline QNull *qnull(void)
> >      return qobject_ref(&qnull_);
> >  }
> >
> > +static inline void qnull_unref(QNull *q) {
> > +    qobject_unref(q);
> > +}
> > +
> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(QNull, qnull_unref)
> > +
> >  #endif /* QNULL_H */
> > diff --git a/include/qapi/qmp/qnum.h b/include/qapi/qmp/qnum.h
> > index 7f84e20bfb2c..ebbf9cd5abe8 100644
> > --- a/include/qapi/qmp/qnum.h
> > +++ b/include/qapi/qmp/qnum.h
> > @@ -54,6 +54,12 @@ struct QNum {
> >      } u;
> >  };
> >
> > +static inline void qnum_unref(QNum *q) {
> > +    qobject_unref(q);
> > +}
> > +
> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(QNum, qnum_unref)
> > +
> >  QNum *qnum_from_int(int64_t value);
> >  QNum *qnum_from_uint(uint64_t value);
> >  QNum *qnum_from_double(double value);
> > diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h
> > index 1d8ba469368f..a38d2925d757 100644
> > --- a/include/qapi/qmp/qstring.h
> > +++ b/include/qapi/qmp/qstring.h
> > @@ -20,6 +20,12 @@ struct QString {
> >      const char *string;
> >  };
> >
> > +static inline void qstring_unref(QString *q) {
> > +    qobject_unref(q);
> > +}
> > +
> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(QString, qstring_unref)
> > +
> >  QString *qstring_new(void);
> >  QString *qstring_from_str(const char *str);
> >  QString *qstring_from_substr(const char *str, size_t start, size_t end);
>