meson.build | 3 --- src/qemu/qemu_domain.c | 7 +++++++ src/util/vireventthread.c | 6 ++++++ src/util/viridentity.c | 6 ++++++ src/util/virobject.c | 6 ++++++ 5 files changed, 25 insertions(+), 3 deletions(-)
This reverts commit b3710e9a2af402a2b620de570b062294e11190eb.
That check is very valuable for our code, but it causes issue with glib >=
2.67.0 when building with clang.
The reason is a combination of two commits in glib, firstly fdda405b6b1b which
adds a g_atomic_pointer_{set,get} variants that enforce stricter type
checking (by removing an extra cast) for compilers that support __typeof__, and
commit dce24dc4492d which effectively enabled the new variant of glib's atomic
code for clang. This will not be necessary when glib's issue #600 [0] (8 years
old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this
issue was opened 3 weeks ago, so there is a slight sliver of hope.
[0] https://gitlab.gnome.org/GNOME/glib/-/issues/600
[1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
---
meson.build | 3 ---
src/qemu/qemu_domain.c | 7 +++++++
src/util/vireventthread.c | 6 ++++++
src/util/viridentity.c | 6 ++++++
src/util/virobject.c | 6 ++++++
5 files changed, 25 insertions(+), 3 deletions(-)
diff --git a/meson.build b/meson.build
index cecaad199d4c..04646e3a078c 100644
--- a/meson.build
+++ b/meson.build
@@ -405,9 +405,6 @@ cc_flags += [
# so use this Clang-specific arg to keep it quiet
'-Wno-typedef-redefinition',
- # Clang complains about casts in G_DEFINE_TYPE(...)
- '-Wno-incompatible-pointer-types-discards-qualifiers',
-
# We don't use -Wc++-compat so we have to enable it explicitly
'-Wjump-misses-init',
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 2158080a56ad..3349476cceae 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -330,7 +330,14 @@ struct _qemuDomainLogContext {
virLogManagerPtr manager;
};
+#pragma clang diagnostic push
+/* Workaround for glib's issue #600 on clang */
+#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers"
+
G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT);
+
+#pragma clang diagnostic pop
+
static virClassPtr qemuDomainSaveCookieClass;
static void qemuDomainLogContextFinalize(GObject *obj);
diff --git a/src/util/vireventthread.c b/src/util/vireventthread.c
index 8342f420f666..4e202c94d0f0 100644
--- a/src/util/vireventthread.c
+++ b/src/util/vireventthread.c
@@ -32,8 +32,14 @@ struct _virEventThread {
GMainLoop *loop;
};
+#pragma clang diagnostic push
+/* Workaround for glib's issue #600 on clang */
+#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers"
+
G_DEFINE_TYPE(virEventThread, vir_event_thread, G_TYPE_OBJECT)
+#pragma clang diagnostic pop
+
#define VIR_FROM_THIS VIR_FROM_EVENT
static void
diff --git a/src/util/viridentity.c b/src/util/viridentity.c
index 2cb9042a846a..43f447f629fe 100644
--- a/src/util/viridentity.c
+++ b/src/util/viridentity.c
@@ -50,8 +50,14 @@ struct _virIdentity {
virTypedParameterPtr params;
};
+#pragma clang diagnostic push
+/* Workaround for glib's issue #600 on clang */
+#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers"
+
G_DEFINE_TYPE(virIdentity, vir_identity, G_TYPE_OBJECT)
+#pragma clang diagnostic pop
+
static virThreadLocal virIdentityCurrent;
static void virIdentityFinalize(GObject *obj);
diff --git a/src/util/virobject.c b/src/util/virobject.c
index a6305354c35e..8d1f26618334 100644
--- a/src/util/virobject.c
+++ b/src/util/virobject.c
@@ -53,8 +53,14 @@ struct _virObjectPrivate {
};
+#pragma clang diagnostic push
+/* Workaround for glib's issue #600 on clang */
+#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers"
+
G_DEFINE_TYPE_WITH_PRIVATE(virObject, vir_object, G_TYPE_OBJECT)
+#pragma clang diagnostic pop
+
#define VIR_OBJECT_NOTVALID(obj) (!obj || !VIR_IS_OBJECT(obj))
#define VIR_OBJECT_USAGE_PRINT_WARNING(anyobj, objclass) \
--
2.29.2
On Sun, Nov 15, 2020 at 09:42:40PM +0100, Martin Kletzander wrote: > This reverts commit b3710e9a2af402a2b620de570b062294e11190eb. > > That check is very valuable for our code, but it causes issue with glib >= > 2.67.0 when building with clang. > > The reason is a combination of two commits in glib, firstly fdda405b6b1b which > adds a g_atomic_pointer_{set,get} variants that enforce stricter type > checking (by removing an extra cast) for compilers that support __typeof__, and > commit dce24dc4492d which effectively enabled the new variant of glib's atomic > code for clang. This will not be necessary when glib's issue #600 [0] (8 years > old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this > issue was opened 3 weeks ago, so there is a slight sliver of hope. > > [0] https://gitlab.gnome.org/GNOME/glib/-/issues/600 > [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719 > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com> > --- > meson.build | 3 --- > src/qemu/qemu_domain.c | 7 +++++++ > src/util/vireventthread.c | 6 ++++++ > src/util/viridentity.c | 6 ++++++ > src/util/virobject.c | 6 ++++++ > 5 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/meson.build b/meson.build > index cecaad199d4c..04646e3a078c 100644 > --- a/meson.build > +++ b/meson.build > @@ -405,9 +405,6 @@ cc_flags += [ > # so use this Clang-specific arg to keep it quiet > '-Wno-typedef-redefinition', > > - # Clang complains about casts in G_DEFINE_TYPE(...) > - '-Wno-incompatible-pointer-types-discards-qualifiers', > - > # We don't use -Wc++-compat so we have to enable it explicitly > '-Wjump-misses-init', > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 2158080a56ad..3349476cceae 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -330,7 +330,14 @@ struct _qemuDomainLogContext { > virLogManagerPtr manager; > }; > > +#pragma clang diagnostic push > +/* Workaround for glib's issue #600 on clang */ > +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" > + Instead of repeating these two macros you can create our own in src/internal.h like we do for other ignores. Otherwise looks good. Pavel
On Mon, Nov 16, 2020 at 09:21:23AM +0100, Pavel Hrdina wrote: >On Sun, Nov 15, 2020 at 09:42:40PM +0100, Martin Kletzander wrote: >> This reverts commit b3710e9a2af402a2b620de570b062294e11190eb. >> >> That check is very valuable for our code, but it causes issue with glib >= >> 2.67.0 when building with clang. >> >> The reason is a combination of two commits in glib, firstly fdda405b6b1b which >> adds a g_atomic_pointer_{set,get} variants that enforce stricter type >> checking (by removing an extra cast) for compilers that support __typeof__, and >> commit dce24dc4492d which effectively enabled the new variant of glib's atomic >> code for clang. This will not be necessary when glib's issue #600 [0] (8 years >> old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this >> issue was opened 3 weeks ago, so there is a slight sliver of hope. >> >> [0] https://gitlab.gnome.org/GNOME/glib/-/issues/600 >> [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719 >> >> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> >> --- >> meson.build | 3 --- >> src/qemu/qemu_domain.c | 7 +++++++ >> src/util/vireventthread.c | 6 ++++++ >> src/util/viridentity.c | 6 ++++++ >> src/util/virobject.c | 6 ++++++ >> 5 files changed, 25 insertions(+), 3 deletions(-) >> >> diff --git a/meson.build b/meson.build >> index cecaad199d4c..04646e3a078c 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -405,9 +405,6 @@ cc_flags += [ >> # so use this Clang-specific arg to keep it quiet >> '-Wno-typedef-redefinition', >> >> - # Clang complains about casts in G_DEFINE_TYPE(...) >> - '-Wno-incompatible-pointer-types-discards-qualifiers', >> - >> # We don't use -Wc++-compat so we have to enable it explicitly >> '-Wjump-misses-init', >> >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index 2158080a56ad..3349476cceae 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -330,7 +330,14 @@ struct _qemuDomainLogContext { >> virLogManagerPtr manager; >> }; >> >> +#pragma clang diagnostic push >> +/* Workaround for glib's issue #600 on clang */ >> +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" >> + > >Instead of repeating these two macros you can create our own in >src/internal.h like we do for other ignores. > Oh, I did not know about using _Pragma in macros. I initialy wanted to make a wrapper macro for the G_DEFINE_TYPE itself, something like: #define G_DEFINE_TYPE_WITH_WORKAROUND(a, b, c) \ _Pragma ("clang diagnostic push") \ _Pragma ("clang diagnostic ignored \"-Wincompatible-pointer-types-discards-qualifiers\"") \ G_DEFINE_TYPE(a, b, c) \ _Pragma ("clang diagnostic pop") But I'm not sure this is something that would not go against other people's ideas. >Otherwise looks good. > >Pavel
On Sun, Nov 15, 2020 at 09:42:40PM +0100, Martin Kletzander wrote: > This reverts commit b3710e9a2af402a2b620de570b062294e11190eb. > > That check is very valuable for our code, but it causes issue with glib >= > 2.67.0 when building with clang. > > The reason is a combination of two commits in glib, firstly fdda405b6b1b which > adds a g_atomic_pointer_{set,get} variants that enforce stricter type > checking (by removing an extra cast) for compilers that support __typeof__, and > commit dce24dc4492d which effectively enabled the new variant of glib's atomic > code for clang. This will not be necessary when glib's issue #600 [0] (8 years > old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this > issue was opened 3 weeks ago, so there is a slight sliver of hope. > > [0] https://gitlab.gnome.org/GNOME/glib/-/issues/600 > [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719 > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com> > --- > meson.build | 3 --- > src/qemu/qemu_domain.c | 7 +++++++ > src/util/vireventthread.c | 6 ++++++ > src/util/viridentity.c | 6 ++++++ > src/util/virobject.c | 6 ++++++ > 5 files changed, 25 insertions(+), 3 deletions(-) > > diff --git a/meson.build b/meson.build > index cecaad199d4c..04646e3a078c 100644 > --- a/meson.build > +++ b/meson.build > @@ -405,9 +405,6 @@ cc_flags += [ > # so use this Clang-specific arg to keep it quiet > '-Wno-typedef-redefinition', > > - # Clang complains about casts in G_DEFINE_TYPE(...) > - '-Wno-incompatible-pointer-types-discards-qualifiers', > - > # We don't use -Wc++-compat so we have to enable it explicitly > '-Wjump-misses-init', > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 2158080a56ad..3349476cceae 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -330,7 +330,14 @@ struct _qemuDomainLogContext { > virLogManagerPtr manager; > }; > > +#pragma clang diagnostic push > +/* Workaround for glib's issue #600 on clang */ > +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" > + > G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT); > + > +#pragma clang diagnostic pop I really don't want to see this pattern used. We may only have a handful of G_DEFINE_TYPE today, but as we convert all 200 virClass over to GObject this is going to be way to tedious. This has to be done as a global thing IMHO. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Nov 16, 2020 at 09:24:22AM +0000, Daniel P. Berrangé wrote: >On Sun, Nov 15, 2020 at 09:42:40PM +0100, Martin Kletzander wrote: >> This reverts commit b3710e9a2af402a2b620de570b062294e11190eb. >> >> That check is very valuable for our code, but it causes issue with glib >= >> 2.67.0 when building with clang. >> >> The reason is a combination of two commits in glib, firstly fdda405b6b1b which >> adds a g_atomic_pointer_{set,get} variants that enforce stricter type >> checking (by removing an extra cast) for compilers that support __typeof__, and >> commit dce24dc4492d which effectively enabled the new variant of glib's atomic >> code for clang. This will not be necessary when glib's issue #600 [0] (8 years >> old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this >> issue was opened 3 weeks ago, so there is a slight sliver of hope. >> >> [0] https://gitlab.gnome.org/GNOME/glib/-/issues/600 >> [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719 >> >> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> >> --- >> meson.build | 3 --- >> src/qemu/qemu_domain.c | 7 +++++++ >> src/util/vireventthread.c | 6 ++++++ >> src/util/viridentity.c | 6 ++++++ >> src/util/virobject.c | 6 ++++++ >> 5 files changed, 25 insertions(+), 3 deletions(-) >> >> diff --git a/meson.build b/meson.build >> index cecaad199d4c..04646e3a078c 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -405,9 +405,6 @@ cc_flags += [ >> # so use this Clang-specific arg to keep it quiet >> '-Wno-typedef-redefinition', >> >> - # Clang complains about casts in G_DEFINE_TYPE(...) >> - '-Wno-incompatible-pointer-types-discards-qualifiers', >> - >> # We don't use -Wc++-compat so we have to enable it explicitly >> '-Wjump-misses-init', >> >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index 2158080a56ad..3349476cceae 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -330,7 +330,14 @@ struct _qemuDomainLogContext { >> virLogManagerPtr manager; >> }; >> >> +#pragma clang diagnostic push >> +/* Workaround for glib's issue #600 on clang */ >> +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" >> + >> G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT); >> + >> +#pragma clang diagnostic pop > >I really don't want to see this pattern used. We may only have a handful >of G_DEFINE_TYPE today, but as we convert all 200 virClass over to GObject >this is going to be way to tedious. This has to be done as a global >thing IMHO. > Even when the warning we're disabling actually catches errors we make every now and then? Really? I know the issue is very old and it only showed up now. And that the MR might not go through for quite some time. But how about a middle ground like the one I described in reply to Pavel? Looks like this: #define G_DEFINE_TYPE_WITH_WORKAROUND(a, b, c) \ _Pragma ("clang diagnostic push") \ _Pragma ("clang diagnostic ignored \"-Wincompatible-pointer-types-discards-qualifiers\"") \ G_DEFINE_TYPE(a, b, c) \ _Pragma ("clang diagnostic pop") If someone wants to make sure it does not break and we really want to get rid of this ASAP, then we can even syntax-check for it, describe it and only use it with glib that has this issue. > >Regards, >Daniel >-- >|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| >|: https://libvirt.org -o- https://fstop138.berrange.com :| >|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Nov 16, 2020 at 10:52:54AM +0100, Martin Kletzander wrote: > On Mon, Nov 16, 2020 at 09:24:22AM +0000, Daniel P. Berrangé wrote: > > On Sun, Nov 15, 2020 at 09:42:40PM +0100, Martin Kletzander wrote: > > > This reverts commit b3710e9a2af402a2b620de570b062294e11190eb. > > > > > > That check is very valuable for our code, but it causes issue with glib >= > > > 2.67.0 when building with clang. > > > > > > The reason is a combination of two commits in glib, firstly fdda405b6b1b which > > > adds a g_atomic_pointer_{set,get} variants that enforce stricter type > > > checking (by removing an extra cast) for compilers that support __typeof__, and > > > commit dce24dc4492d which effectively enabled the new variant of glib's atomic > > > code for clang. This will not be necessary when glib's issue #600 [0] (8 years > > > old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this > > > issue was opened 3 weeks ago, so there is a slight sliver of hope. > > > > > > [0] https://gitlab.gnome.org/GNOME/glib/-/issues/600 > > > [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719 > > > > > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com> > > > --- > > > meson.build | 3 --- > > > src/qemu/qemu_domain.c | 7 +++++++ > > > src/util/vireventthread.c | 6 ++++++ > > > src/util/viridentity.c | 6 ++++++ > > > src/util/virobject.c | 6 ++++++ > > > 5 files changed, 25 insertions(+), 3 deletions(-) > > > > > > diff --git a/meson.build b/meson.build > > > index cecaad199d4c..04646e3a078c 100644 > > > --- a/meson.build > > > +++ b/meson.build > > > @@ -405,9 +405,6 @@ cc_flags += [ > > > # so use this Clang-specific arg to keep it quiet > > > '-Wno-typedef-redefinition', > > > > > > - # Clang complains about casts in G_DEFINE_TYPE(...) > > > - '-Wno-incompatible-pointer-types-discards-qualifiers', > > > - > > > # We don't use -Wc++-compat so we have to enable it explicitly > > > '-Wjump-misses-init', > > > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > > > index 2158080a56ad..3349476cceae 100644 > > > --- a/src/qemu/qemu_domain.c > > > +++ b/src/qemu/qemu_domain.c > > > @@ -330,7 +330,14 @@ struct _qemuDomainLogContext { > > > virLogManagerPtr manager; > > > }; > > > > > > +#pragma clang diagnostic push > > > +/* Workaround for glib's issue #600 on clang */ > > > +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" > > > + > > > G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT); > > > + > > > +#pragma clang diagnostic pop > > > > I really don't want to see this pattern used. We may only have a handful > > of G_DEFINE_TYPE today, but as we convert all 200 virClass over to GObject > > this is going to be way to tedious. This has to be done as a global > > thing IMHO. > > > > Even when the warning we're disabling actually catches errors we make every now > and then? Really? You can disable it globall, but only when compiling against the glib versions affected. We'll still get coverage of the warning flag when building against other versions. > > I know the issue is very old and it only showed up now. And that the MR might > not go through for quite some time. But how about a middle ground like the one > I described in reply to Pavel? Looks like this: > > #define G_DEFINE_TYPE_WITH_WORKAROUND(a, b, c) \ > _Pragma ("clang diagnostic push") \ > _Pragma ("clang diagnostic ignored \"-Wincompatible-pointer-types-discards-qualifiers\"") \ > G_DEFINE_TYPE(a, b, c) \ > _Pragma ("clang diagnostic pop") > > If someone wants to make sure it does not break and we really want to get rid of > this ASAP, then we can even syntax-check for it, describe it and only use it > with glib that has this issue. That can't be done selectively - we would have to use that wrapper macro unconditionally, so it'd be present for years until we bump the min glib to a version that has the fix. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 11/16/20 10:59 AM, Daniel P. Berrangé wrote: > On Mon, Nov 16, 2020 at 10:52:54AM +0100, Martin Kletzander wrote: >> On Mon, Nov 16, 2020 at 09:24:22AM +0000, Daniel P. Berrangé wrote: >>> On Sun, Nov 15, 2020 at 09:42:40PM +0100, Martin Kletzander wrote: >>>> This reverts commit b3710e9a2af402a2b620de570b062294e11190eb. >>>> >>>> That check is very valuable for our code, but it causes issue with glib >= >>>> 2.67.0 when building with clang. >>>> >>>> The reason is a combination of two commits in glib, firstly fdda405b6b1b which >>>> adds a g_atomic_pointer_{set,get} variants that enforce stricter type >>>> checking (by removing an extra cast) for compilers that support __typeof__, and >>>> commit dce24dc4492d which effectively enabled the new variant of glib's atomic >>>> code for clang. This will not be necessary when glib's issue #600 [0] (8 years >>>> old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this >>>> issue was opened 3 weeks ago, so there is a slight sliver of hope. >>>> >>>> [0] https://gitlab.gnome.org/GNOME/glib/-/issues/600 >>>> [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719 >>>> >>>> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> >>>> --- >>>> meson.build | 3 --- >>>> src/qemu/qemu_domain.c | 7 +++++++ >>>> src/util/vireventthread.c | 6 ++++++ >>>> src/util/viridentity.c | 6 ++++++ >>>> src/util/virobject.c | 6 ++++++ >>>> 5 files changed, 25 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/meson.build b/meson.build >>>> index cecaad199d4c..04646e3a078c 100644 >>>> --- a/meson.build >>>> +++ b/meson.build >>>> @@ -405,9 +405,6 @@ cc_flags += [ >>>> # so use this Clang-specific arg to keep it quiet >>>> '-Wno-typedef-redefinition', >>>> >>>> - # Clang complains about casts in G_DEFINE_TYPE(...) >>>> - '-Wno-incompatible-pointer-types-discards-qualifiers', >>>> - >>>> # We don't use -Wc++-compat so we have to enable it explicitly >>>> '-Wjump-misses-init', >>>> >>>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >>>> index 2158080a56ad..3349476cceae 100644 >>>> --- a/src/qemu/qemu_domain.c >>>> +++ b/src/qemu/qemu_domain.c >>>> @@ -330,7 +330,14 @@ struct _qemuDomainLogContext { >>>> virLogManagerPtr manager; >>>> }; >>>> >>>> +#pragma clang diagnostic push >>>> +/* Workaround for glib's issue #600 on clang */ >>>> +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" >>>> + >>>> G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT); >>>> + >>>> +#pragma clang diagnostic pop >>> >>> I really don't want to see this pattern used. We may only have a handful >>> of G_DEFINE_TYPE today, but as we convert all 200 virClass over to GObject >>> this is going to be way to tedious. This has to be done as a global >>> thing IMHO. >>> >> >> Even when the warning we're disabling actually catches errors we make every now >> and then? Really? > > You can disable it globall, but only when compiling against the glib versions > affected. We'll still get coverage of the warning flag when building against > other versions. > >> >> I know the issue is very old and it only showed up now. And that the MR might >> not go through for quite some time. But how about a middle ground like the one >> I described in reply to Pavel? Looks like this: >> >> #define G_DEFINE_TYPE_WITH_WORKAROUND(a, b, c) \ >> _Pragma ("clang diagnostic push") \ >> _Pragma ("clang diagnostic ignored \"-Wincompatible-pointer-types-discards-qualifiers\"") \ >> G_DEFINE_TYPE(a, b, c) \ >> _Pragma ("clang diagnostic pop") >> >> If someone wants to make sure it does not break and we really want to get rid of >> this ASAP, then we can even syntax-check for it, describe it and only use it >> with glib that has this issue. > > That can't be done selectively - we would have to use that wrapper macro > unconditionally, so it'd be present for years until we bump the min glib > to a version that has the fix. So what about something like this: #if glib is broken # undefine G_DEFINE_TYPE # define G_DEFINE_TYPE \ # some pragma magic (basically what martin has above) \ #endif This way we can continue using G_DEFINE_TYPE and ditch this redefine once glib is updated to fixed version. Michal
On Mon, Nov 16, 2020 at 11:26:45AM +0100, Michal Privoznik wrote: > On 11/16/20 10:59 AM, Daniel P. Berrangé wrote: > > On Mon, Nov 16, 2020 at 10:52:54AM +0100, Martin Kletzander wrote: > > > On Mon, Nov 16, 2020 at 09:24:22AM +0000, Daniel P. Berrangé wrote: > > > > On Sun, Nov 15, 2020 at 09:42:40PM +0100, Martin Kletzander wrote: > > > > > This reverts commit b3710e9a2af402a2b620de570b062294e11190eb. > > > > > > > > > > That check is very valuable for our code, but it causes issue with glib >= > > > > > 2.67.0 when building with clang. > > > > > > > > > > The reason is a combination of two commits in glib, firstly fdda405b6b1b which > > > > > adds a g_atomic_pointer_{set,get} variants that enforce stricter type > > > > > checking (by removing an extra cast) for compilers that support __typeof__, and > > > > > commit dce24dc4492d which effectively enabled the new variant of glib's atomic > > > > > code for clang. This will not be necessary when glib's issue #600 [0] (8 years > > > > > old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this > > > > > issue was opened 3 weeks ago, so there is a slight sliver of hope. > > > > > > > > > > [0] https://gitlab.gnome.org/GNOME/glib/-/issues/600 > > > > > [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719 > > > > > > > > > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com> > > > > > --- > > > > > meson.build | 3 --- > > > > > src/qemu/qemu_domain.c | 7 +++++++ > > > > > src/util/vireventthread.c | 6 ++++++ > > > > > src/util/viridentity.c | 6 ++++++ > > > > > src/util/virobject.c | 6 ++++++ > > > > > 5 files changed, 25 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/meson.build b/meson.build > > > > > index cecaad199d4c..04646e3a078c 100644 > > > > > --- a/meson.build > > > > > +++ b/meson.build > > > > > @@ -405,9 +405,6 @@ cc_flags += [ > > > > > # so use this Clang-specific arg to keep it quiet > > > > > '-Wno-typedef-redefinition', > > > > > > > > > > - # Clang complains about casts in G_DEFINE_TYPE(...) > > > > > - '-Wno-incompatible-pointer-types-discards-qualifiers', > > > > > - > > > > > # We don't use -Wc++-compat so we have to enable it explicitly > > > > > '-Wjump-misses-init', > > > > > > > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > > > > > index 2158080a56ad..3349476cceae 100644 > > > > > --- a/src/qemu/qemu_domain.c > > > > > +++ b/src/qemu/qemu_domain.c > > > > > @@ -330,7 +330,14 @@ struct _qemuDomainLogContext { > > > > > virLogManagerPtr manager; > > > > > }; > > > > > > > > > > +#pragma clang diagnostic push > > > > > +/* Workaround for glib's issue #600 on clang */ > > > > > +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" > > > > > + > > > > > G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT); > > > > > + > > > > > +#pragma clang diagnostic pop > > > > > > > > I really don't want to see this pattern used. We may only have a handful > > > > of G_DEFINE_TYPE today, but as we convert all 200 virClass over to GObject > > > > this is going to be way to tedious. This has to be done as a global > > > > thing IMHO. > > > > > > > > > > Even when the warning we're disabling actually catches errors we make every now > > > and then? Really? > > > > You can disable it globall, but only when compiling against the glib versions > > affected. We'll still get coverage of the warning flag when building against > > other versions. > > > > > > > > I know the issue is very old and it only showed up now. And that the MR might > > > not go through for quite some time. But how about a middle ground like the one > > > I described in reply to Pavel? Looks like this: > > > > > > #define G_DEFINE_TYPE_WITH_WORKAROUND(a, b, c) \ > > > _Pragma ("clang diagnostic push") \ > > > _Pragma ("clang diagnostic ignored \"-Wincompatible-pointer-types-discards-qualifiers\"") \ > > > G_DEFINE_TYPE(a, b, c) \ > > > _Pragma ("clang diagnostic pop") > > > > > > If someone wants to make sure it does not break and we really want to get rid of > > > this ASAP, then we can even syntax-check for it, describe it and only use it > > > with glib that has this issue. > > > > That can't be done selectively - we would have to use that wrapper macro > > unconditionally, so it'd be present for years until we bump the min glib > > to a version that has the fix. > > So what about something like this: > > #if glib is broken > # undefine G_DEFINE_TYPE > # define G_DEFINE_TYPE \ > # some pragma magic (basically what martin has above) \ > #endif > > This way we can continue using G_DEFINE_TYPE and ditch this redefine once > glib is updated to fixed version. If you can make that work, that'd be ok as it isolates the workaround in a single place. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Nov 16, 2020 at 10:48:20AM +0000, Daniel P. Berrangé wrote: >On Mon, Nov 16, 2020 at 11:26:45AM +0100, Michal Privoznik wrote: >> On 11/16/20 10:59 AM, Daniel P. Berrangé wrote: >> > On Mon, Nov 16, 2020 at 10:52:54AM +0100, Martin Kletzander wrote: >> > > On Mon, Nov 16, 2020 at 09:24:22AM +0000, Daniel P. Berrangé wrote: >> > > > On Sun, Nov 15, 2020 at 09:42:40PM +0100, Martin Kletzander wrote: >> > > > > This reverts commit b3710e9a2af402a2b620de570b062294e11190eb. >> > > > > >> > > > > That check is very valuable for our code, but it causes issue with glib >= >> > > > > 2.67.0 when building with clang. >> > > > > >> > > > > The reason is a combination of two commits in glib, firstly fdda405b6b1b which >> > > > > adds a g_atomic_pointer_{set,get} variants that enforce stricter type >> > > > > checking (by removing an extra cast) for compilers that support __typeof__, and >> > > > > commit dce24dc4492d which effectively enabled the new variant of glib's atomic >> > > > > code for clang. This will not be necessary when glib's issue #600 [0] (8 years >> > > > > old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this >> > > > > issue was opened 3 weeks ago, so there is a slight sliver of hope. >> > > > > >> > > > > [0] https://gitlab.gnome.org/GNOME/glib/-/issues/600 >> > > > > [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719 >> > > > > >> > > > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com> >> > > > > --- >> > > > > meson.build | 3 --- >> > > > > src/qemu/qemu_domain.c | 7 +++++++ >> > > > > src/util/vireventthread.c | 6 ++++++ >> > > > > src/util/viridentity.c | 6 ++++++ >> > > > > src/util/virobject.c | 6 ++++++ >> > > > > 5 files changed, 25 insertions(+), 3 deletions(-) >> > > > > >> > > > > diff --git a/meson.build b/meson.build >> > > > > index cecaad199d4c..04646e3a078c 100644 >> > > > > --- a/meson.build >> > > > > +++ b/meson.build >> > > > > @@ -405,9 +405,6 @@ cc_flags += [ >> > > > > # so use this Clang-specific arg to keep it quiet >> > > > > '-Wno-typedef-redefinition', >> > > > > >> > > > > - # Clang complains about casts in G_DEFINE_TYPE(...) >> > > > > - '-Wno-incompatible-pointer-types-discards-qualifiers', >> > > > > - >> > > > > # We don't use -Wc++-compat so we have to enable it explicitly >> > > > > '-Wjump-misses-init', >> > > > > >> > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> > > > > index 2158080a56ad..3349476cceae 100644 >> > > > > --- a/src/qemu/qemu_domain.c >> > > > > +++ b/src/qemu/qemu_domain.c >> > > > > @@ -330,7 +330,14 @@ struct _qemuDomainLogContext { >> > > > > virLogManagerPtr manager; >> > > > > }; >> > > > > >> > > > > +#pragma clang diagnostic push >> > > > > +/* Workaround for glib's issue #600 on clang */ >> > > > > +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" >> > > > > + >> > > > > G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT); >> > > > > + >> > > > > +#pragma clang diagnostic pop >> > > > >> > > > I really don't want to see this pattern used. We may only have a handful >> > > > of G_DEFINE_TYPE today, but as we convert all 200 virClass over to GObject >> > > > this is going to be way to tedious. This has to be done as a global >> > > > thing IMHO. >> > > > >> > > >> > > Even when the warning we're disabling actually catches errors we make every now >> > > and then? Really? >> > >> > You can disable it globall, but only when compiling against the glib versions >> > affected. We'll still get coverage of the warning flag when building against >> > other versions. >> > >> > > >> > > I know the issue is very old and it only showed up now. And that the MR might >> > > not go through for quite some time. But how about a middle ground like the one >> > > I described in reply to Pavel? Looks like this: >> > > >> > > #define G_DEFINE_TYPE_WITH_WORKAROUND(a, b, c) \ >> > > _Pragma ("clang diagnostic push") \ >> > > _Pragma ("clang diagnostic ignored \"-Wincompatible-pointer-types-discards-qualifiers\"") \ >> > > G_DEFINE_TYPE(a, b, c) \ >> > > _Pragma ("clang diagnostic pop") >> > > >> > > If someone wants to make sure it does not break and we really want to get rid of >> > > this ASAP, then we can even syntax-check for it, describe it and only use it >> > > with glib that has this issue. >> > >> > That can't be done selectively - we would have to use that wrapper macro >> > unconditionally, so it'd be present for years until we bump the min glib >> > to a version that has the fix. >> >> So what about something like this: >> >> #if glib is broken >> # undefine G_DEFINE_TYPE >> # define G_DEFINE_TYPE \ >> # some pragma magic (basically what martin has above) \ >> #endif >> >> This way we can continue using G_DEFINE_TYPE and ditch this redefine once >> glib is updated to fixed version. > >If you can make that work, that'd be ok as it isolates the workaround in >a single place. > OK, thanks Michal for following up because I was kind of misunderstanding the intention. If we #undef the original G_DEFINE_TYPE we would have to open code it, being prone to errors if the original definition changes. If we change it to e.g. VIR_G_DEFINE_TYPE() (and disable G_DEFINE_TYPE usage out of sr/util/glibcompat.c) then we can just do what I wrote above and whenever it is removed it is easy to see what places in the code need changing. I know you are against that, but I don't see how different that is compared to, for example vir_g_canonicalize_filename(). And other functions in gcompat.h also, although they don't even have the version check. >Regards, >Daniel >-- >|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| >|: https://libvirt.org -o- https://fstop138.berrange.com :| >|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Nov 16, 2020 at 12:45:41PM +0100, Martin Kletzander wrote: > On Mon, Nov 16, 2020 at 10:48:20AM +0000, Daniel P. Berrangé wrote: > > On Mon, Nov 16, 2020 at 11:26:45AM +0100, Michal Privoznik wrote: > > > On 11/16/20 10:59 AM, Daniel P. Berrangé wrote: > > > > On Mon, Nov 16, 2020 at 10:52:54AM +0100, Martin Kletzander wrote: > > > > > On Mon, Nov 16, 2020 at 09:24:22AM +0000, Daniel P. Berrangé wrote: > > > > > > On Sun, Nov 15, 2020 at 09:42:40PM +0100, Martin Kletzander wrote: > > > > > > > This reverts commit b3710e9a2af402a2b620de570b062294e11190eb. > > > > > > > > > > > > > > That check is very valuable for our code, but it causes issue with glib >= > > > > > > > 2.67.0 when building with clang. > > > > > > > > > > > > > > The reason is a combination of two commits in glib, firstly fdda405b6b1b which > > > > > > > adds a g_atomic_pointer_{set,get} variants that enforce stricter type > > > > > > > checking (by removing an extra cast) for compilers that support __typeof__, and > > > > > > > commit dce24dc4492d which effectively enabled the new variant of glib's atomic > > > > > > > code for clang. This will not be necessary when glib's issue #600 [0] (8 years > > > > > > > old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this > > > > > > > issue was opened 3 weeks ago, so there is a slight sliver of hope. > > > > > > > > > > > > > > [0] https://gitlab.gnome.org/GNOME/glib/-/issues/600 > > > > > > > [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719 > > > > > > > > > > > > > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com> > > > > > > > --- > > > > > > > meson.build | 3 --- > > > > > > > src/qemu/qemu_domain.c | 7 +++++++ > > > > > > > src/util/vireventthread.c | 6 ++++++ > > > > > > > src/util/viridentity.c | 6 ++++++ > > > > > > > src/util/virobject.c | 6 ++++++ > > > > > > > 5 files changed, 25 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/meson.build b/meson.build > > > > > > > index cecaad199d4c..04646e3a078c 100644 > > > > > > > --- a/meson.build > > > > > > > +++ b/meson.build > > > > > > > @@ -405,9 +405,6 @@ cc_flags += [ > > > > > > > # so use this Clang-specific arg to keep it quiet > > > > > > > '-Wno-typedef-redefinition', > > > > > > > > > > > > > > - # Clang complains about casts in G_DEFINE_TYPE(...) > > > > > > > - '-Wno-incompatible-pointer-types-discards-qualifiers', > > > > > > > - > > > > > > > # We don't use -Wc++-compat so we have to enable it explicitly > > > > > > > '-Wjump-misses-init', > > > > > > > > > > > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > > > > > > > index 2158080a56ad..3349476cceae 100644 > > > > > > > --- a/src/qemu/qemu_domain.c > > > > > > > +++ b/src/qemu/qemu_domain.c > > > > > > > @@ -330,7 +330,14 @@ struct _qemuDomainLogContext { > > > > > > > virLogManagerPtr manager; > > > > > > > }; > > > > > > > > > > > > > > +#pragma clang diagnostic push > > > > > > > +/* Workaround for glib's issue #600 on clang */ > > > > > > > +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" > > > > > > > + > > > > > > > G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT); > > > > > > > + > > > > > > > +#pragma clang diagnostic pop > > > > > > > > > > > > I really don't want to see this pattern used. We may only have a handful > > > > > > of G_DEFINE_TYPE today, but as we convert all 200 virClass over to GObject > > > > > > this is going to be way to tedious. This has to be done as a global > > > > > > thing IMHO. > > > > > > > > > > > > > > > > Even when the warning we're disabling actually catches errors we make every now > > > > > and then? Really? > > > > > > > > You can disable it globall, but only when compiling against the glib versions > > > > affected. We'll still get coverage of the warning flag when building against > > > > other versions. > > > > > > > > > > > > > > I know the issue is very old and it only showed up now. And that the MR might > > > > > not go through for quite some time. But how about a middle ground like the one > > > > > I described in reply to Pavel? Looks like this: > > > > > > > > > > #define G_DEFINE_TYPE_WITH_WORKAROUND(a, b, c) \ > > > > > _Pragma ("clang diagnostic push") \ > > > > > _Pragma ("clang diagnostic ignored \"-Wincompatible-pointer-types-discards-qualifiers\"") \ > > > > > G_DEFINE_TYPE(a, b, c) \ > > > > > _Pragma ("clang diagnostic pop") > > > > > > > > > > If someone wants to make sure it does not break and we really want to get rid of > > > > > this ASAP, then we can even syntax-check for it, describe it and only use it > > > > > with glib that has this issue. > > > > > > > > That can't be done selectively - we would have to use that wrapper macro > > > > unconditionally, so it'd be present for years until we bump the min glib > > > > to a version that has the fix. > > > > > > So what about something like this: > > > > > > #if glib is broken > > > # undefine G_DEFINE_TYPE > > > # define G_DEFINE_TYPE \ > > > # some pragma magic (basically what martin has above) \ > > > #endif > > > > > > This way we can continue using G_DEFINE_TYPE and ditch this redefine once > > > glib is updated to fixed version. > > > > If you can make that work, that'd be ok as it isolates the workaround in > > a single place. > > > > OK, thanks Michal for following up because I was kind of misunderstanding the > intention. > > If we #undef the original G_DEFINE_TYPE we would have to open code it, being > prone to errors if the original definition changes. If we change it to > e.g. VIR_G_DEFINE_TYPE() (and disable G_DEFINE_TYPE usage out of > sr/util/glibcompat.c) then we can just do what I wrote above and whenever it is > removed it is easy to see what places in the code need changing. I know you are > against that, but I don't see how different that is compared to, for example > vir_g_canonicalize_filename(). And other functions in gcompat.h also, although > they don't even have the version check. No code ever calls vir_g_canonicalize_filename directly, it continues to use g_canonicalize_filename as normal. The calls get transparently re-written to vir_g_canonicalize_filename - basically the same hack that gnulib used to transparently replace functions. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Nov 16, 2020 at 12:45:41PM +0100, Martin Kletzander wrote: >On Mon, Nov 16, 2020 at 10:48:20AM +0000, Daniel P. Berrangé wrote: >>On Mon, Nov 16, 2020 at 11:26:45AM +0100, Michal Privoznik wrote: >>> On 11/16/20 10:59 AM, Daniel P. Berrangé wrote: >>> > On Mon, Nov 16, 2020 at 10:52:54AM +0100, Martin Kletzander wrote: >>> > > On Mon, Nov 16, 2020 at 09:24:22AM +0000, Daniel P. Berrangé wrote: >>> > > > On Sun, Nov 15, 2020 at 09:42:40PM +0100, Martin Kletzander wrote: >>> > > > > This reverts commit b3710e9a2af402a2b620de570b062294e11190eb. >>> > > > > >>> > > > > That check is very valuable for our code, but it causes issue with glib >= >>> > > > > 2.67.0 when building with clang. >>> > > > > >>> > > > > The reason is a combination of two commits in glib, firstly fdda405b6b1b which >>> > > > > adds a g_atomic_pointer_{set,get} variants that enforce stricter type >>> > > > > checking (by removing an extra cast) for compilers that support __typeof__, and >>> > > > > commit dce24dc4492d which effectively enabled the new variant of glib's atomic >>> > > > > code for clang. This will not be necessary when glib's issue #600 [0] (8 years >>> > > > > old) is fixed. Thankfully, MR #1719 [1], which is supposed to deal with this >>> > > > > issue was opened 3 weeks ago, so there is a slight sliver of hope. >>> > > > > >>> > > > > [0] https://gitlab.gnome.org/GNOME/glib/-/issues/600 >>> > > > > [1] https://gitlab.gnome.org/GNOME/glib/-/merge_requests/1719 >>> > > > > >>> > > > > Signed-off-by: Martin Kletzander <mkletzan@redhat.com> >>> > > > > --- >>> > > > > meson.build | 3 --- >>> > > > > src/qemu/qemu_domain.c | 7 +++++++ >>> > > > > src/util/vireventthread.c | 6 ++++++ >>> > > > > src/util/viridentity.c | 6 ++++++ >>> > > > > src/util/virobject.c | 6 ++++++ >>> > > > > 5 files changed, 25 insertions(+), 3 deletions(-) >>> > > > > >>> > > > > diff --git a/meson.build b/meson.build >>> > > > > index cecaad199d4c..04646e3a078c 100644 >>> > > > > --- a/meson.build >>> > > > > +++ b/meson.build >>> > > > > @@ -405,9 +405,6 @@ cc_flags += [ >>> > > > > # so use this Clang-specific arg to keep it quiet >>> > > > > '-Wno-typedef-redefinition', >>> > > > > >>> > > > > - # Clang complains about casts in G_DEFINE_TYPE(...) >>> > > > > - '-Wno-incompatible-pointer-types-discards-qualifiers', >>> > > > > - >>> > > > > # We don't use -Wc++-compat so we have to enable it explicitly >>> > > > > '-Wjump-misses-init', >>> > > > > >>> > > > > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >>> > > > > index 2158080a56ad..3349476cceae 100644 >>> > > > > --- a/src/qemu/qemu_domain.c >>> > > > > +++ b/src/qemu/qemu_domain.c >>> > > > > @@ -330,7 +330,14 @@ struct _qemuDomainLogContext { >>> > > > > virLogManagerPtr manager; >>> > > > > }; >>> > > > > >>> > > > > +#pragma clang diagnostic push >>> > > > > +/* Workaround for glib's issue #600 on clang */ >>> > > > > +#pragma clang diagnostic ignored "-Wincompatible-pointer-types-discards-qualifiers" >>> > > > > + >>> > > > > G_DEFINE_TYPE(qemuDomainLogContext, qemu_domain_log_context, G_TYPE_OBJECT); >>> > > > > + >>> > > > > +#pragma clang diagnostic pop >>> > > > >>> > > > I really don't want to see this pattern used. We may only have a handful >>> > > > of G_DEFINE_TYPE today, but as we convert all 200 virClass over to GObject >>> > > > this is going to be way to tedious. This has to be done as a global >>> > > > thing IMHO. >>> > > > >>> > > >>> > > Even when the warning we're disabling actually catches errors we make every now >>> > > and then? Really? >>> > >>> > You can disable it globall, but only when compiling against the glib versions >>> > affected. We'll still get coverage of the warning flag when building against >>> > other versions. >>> > >>> > > >>> > > I know the issue is very old and it only showed up now. And that the MR might >>> > > not go through for quite some time. But how about a middle ground like the one >>> > > I described in reply to Pavel? Looks like this: >>> > > >>> > > #define G_DEFINE_TYPE_WITH_WORKAROUND(a, b, c) \ >>> > > _Pragma ("clang diagnostic push") \ >>> > > _Pragma ("clang diagnostic ignored \"-Wincompatible-pointer-types-discards-qualifiers\"") \ >>> > > G_DEFINE_TYPE(a, b, c) \ >>> > > _Pragma ("clang diagnostic pop") >>> > > >>> > > If someone wants to make sure it does not break and we really want to get rid of >>> > > this ASAP, then we can even syntax-check for it, describe it and only use it >>> > > with glib that has this issue. >>> > >>> > That can't be done selectively - we would have to use that wrapper macro >>> > unconditionally, so it'd be present for years until we bump the min glib >>> > to a version that has the fix. >>> >>> So what about something like this: >>> >>> #if glib is broken >>> # undefine G_DEFINE_TYPE >>> # define G_DEFINE_TYPE \ >>> # some pragma magic (basically what martin has above) \ >>> #endif >>> >>> This way we can continue using G_DEFINE_TYPE and ditch this redefine once >>> glib is updated to fixed version. >> >>If you can make that work, that'd be ok as it isolates the workaround in >>a single place. >> > >OK, thanks Michal for following up because I was kind of misunderstanding the >intention. > >If we #undef the original G_DEFINE_TYPE we would have to open code it, being >prone to errors if the original definition changes. If we change it to >e.g. VIR_G_DEFINE_TYPE() (and disable G_DEFINE_TYPE usage out of >sr/util/glibcompat.c) then we can just do what I wrote above and whenever it is >removed it is easy to see what places in the code need changing. I know you are >against that, but I don't see how different that is compared to, for example >vir_g_canonicalize_filename(). And other functions in gcompat.h also, although >they don't even have the version check. > OK, my bad, gcompat.h makes all the difference. I'll see what I can come up with. >>Regards, >>Daniel >>-- >>|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| >>|: https://libvirt.org -o- https://fstop138.berrange.com :| >>|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
© 2016 - 2024 Red Hat, Inc.