Re: [libvirt PATCH v2 01/12] glibcompat: Add G_GNUC_UNUSED to g_auto* definitions for clang
On Fri, 2021-09-10 at 13:33 +0100, Daniel P. Berrangé wrote:
On Fri, Sep 10, 2021 at 02:26:32PM +0200, Tim Wiederhake wrote:
> On Fri, 2021-09-10 at 11:53 +0100, Daniel P. Berrangé wrote:
> > On Fri, Sep 10, 2021 at 12:45:43PM +0200, Tim Wiederhake wrote:
> > > Workaround for a bug in clang. Clang emits an unused-variable
> > > warning
> > > if the variable is only accessed on scope exit by a destructor
> > > function.
> > > Note that gcc does not exhibit this behavior.
> > >
> > > See https://bugs.llvm.org/show_bug.cgi?id=3888 and
> > > https://bugs.llvm.org/show_bug.cgi?id=43482.
> > >
> > > Signed-off-by: Tim Wiederhake <twiederh(a)redhat.com>
> > > ---
> > > src/util/glibcompat.h | 19 +++++++++++++++++++
> > > 1 file changed, 19 insertions(+)
> > >
> > > diff --git a/src/util/glibcompat.h b/src/util/glibcompat.h
> > > index 697687b967..ea486e32d5 100644
> > > --- a/src/util/glibcompat.h
> > > +++ b/src/util/glibcompat.h
> > > @@ -86,3 +86,22 @@ char *vir_g_strdup_vprintf(const char *msg,
> > > va_list args)
> > > #define g_fsync vir_g_fsync
> > >
> > > void vir_g_source_unref(GSource *src, GMainContext *ctx);
> > > +
> > > +/*
> > > + * Workaround for a bug in clang. Clang emits an unused-variable
> > > warning if the
> > > + * variable is only accessed on scope exit by a destructor
> > > function. See
> > > + * https://bugs.llvm.org/show_bug.cgi?id=3888 and
> > > + * https://bugs.llvm.org/show_bug.cgi?id=43482.
> > > + */
> > > +#if defined(__clang__)
> > > +# undef g_autoptr
> > > +# define g_autoptr(TypeName)
> > > _GLIB_CLEANUP(_GLIB_AUTOPTR_FUNC_NAME(TypeName))
> > > _GLIB_AUTOPTR_TYPENAME(TypeName) G_GNUC_UNUSED
> > > +# undef g_autolist
> > > +# define g_autolist(TypeName)
> > > _GLIB_CLEANUP(_GLIB_AUTOPTR_LIST_FUNC_NAME(TypeName))
> > > _GLIB_AUTOPTR_LIST_TYPENAME(TypeName) G_GNUC_UNUSED
> > > +# undef g_autoslist
> > > +# define g_autoslist(TypeName)
> > > _GLIB_CLEANUP(_GLIB_AUTOPTR_SLIST_FUNC_NAME(TypeName))
> > > _GLIB_AUTOPTR_SLIST_TYPENAME(TypeName) G_GNUC_UNUSED
> > > +# undef g_autoqueue
> > > +# define g_autoqueue(TypeName)
> > > _GLIB_CLEANUP(_GLIB_AUTOPTR_QUEUE_FUNC_NAME(TypeName))
> > > _GLIB_AUTOPTR_QUEUE_TYPENAME(TypeName) G_GNUC_UNUSED
> > > +# undef g_auto
> > > +# define g_auto(TypeName)
> > > _GLIB_CLEANUP(_GLIB_AUTO_FUNC_NAME(TypeName)) TypeName
> > > G_GNUC_UNUSED
> > > +#endif /* __clang__ */
> >
> > This patch should not be needed anymore afaik since we addressed
> > the
> > problems that were triggering clang.
> >
>
> Did you see patches 9 to 12? They introduce more "only used on scope
> exit" variables that trigger clangs warning otherwise.
Why aren't they following the approach that Peter used to address this
though, by adding the G_GNUC_UNUSED annotations to the usage sites ?
Adding G_GNUC_UNUSED at the usage sites (of which there will be a
couple hundred), will disable unused variable detection irregardless of
used compiler, just to work around a bug in one single compiler.
I would rather use "G_GNUC_UNUSED" only for variables / function
arguments that actually are unused. The lock guard variables are used,
but at the end of the scope rather than during.
We should only re-define the g_auto* macros if we have first
submitted
this change to GLib upstream and they merged it.
What are your thoughts on macros (#if/#else'd over __clang__ to include
G_GNUC_UNUSED as required) like this?
#define VIR_LOCK_GUARD_MUTEX(mutex) \
g_autoptr(virLockGuard) G_GNUC_UNUSED lockguard ## __COUNTER__ =
virLockGuardNew(mutex);
#define VIR_LOCK_GUARD_OBJECT(object) \
g_autoptr(virLockGuard) G_GNUC_UNUSED lockguard ## __COUNTER__ =
virObjectLockGuard(object);
or:
#define VIR_LOCK_GUARD_MUTEX g_autoptr(virLockGuard) G_GNUC_UNUSED
VIR_LOCK_GUARD_MUTEX lock1 = virLockGuardNew(mutex);
VIR_LOCK_GUARD_MUTEX lock2 = virObjectLockGuard(object);
Regards,
Tim
Regards,
Daniel