On Thu, Jun 07, 2018 at 10:57:56AM +0200, Thomas Huth wrote:
> On 06.06.2018 19:32, Daniel P. Berrangé wrote:
> > There are two useful macros that can be defined before including
> > glib.h that are related to the min required glib version
> >
> > - GLIB_VERSION_MIN_REQUIRED
> >
> > When this is defined, if code uses an API that was deprecated
> > in this version, or older, a compiler warning will be emitted.
> > This alerts maintainers to update their code to whatever new
> > replacement API is now recommended best practice.
> >
> > - GLIB_VERSION_MAX_ALLOWED
> >
> > When this is defined, if code uses an API that was introduced
> > in a version that is newer than the declared version, a compiler
> > warning will be emitted. This alerts maintainers if new code
> > accidentally uses functionality that won't be available on some
> > supported platforms.
> >
> > The GLIB_VERSION_MAX_ALLOWED constant makes it a bit harder to opt
> > in to using specific new APIs with a GLIB_CHECK_VERSION conditional.
> > To workaround this Pragmas can be used to temporarily turn off the
> > -Wdeprecated-declarations compiler warning, while a static inline
> > compat function is implemented. This workaround is illustrated with the
> > implementation of the g_strv_contains method to satisfy the test suite.
>
> I wonder whether that's a little bit too much magic already. We could
> also set GLIB_VERSION_MAX_ALLOWED to the version where we've already got
> the work-arounds in place (i.e. to 2.44 here). If we introduce
> additional code that uses other new functions from 2.41 - 2.44, this
> should also be caught by the patchew CI builders?
I don't think that's a good direction to take - if we follow that
through and someone backports a function from 2.56, we'll loose
100% of the benefit of the MAX_ALLOWED checking. Given we only have
a handful of backported functions, that is not a good tradeoff.
> > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
> > ---
> > include/glib-compat.h | 67 ++++++++++++++++++++++++
> > tests/docker/dockerfiles/centos6.docker | 30 -----------
> > tests/docker/dockerfiles/min-glib.docker | 8 ---
>
> The docker files are not directly related to this patch, are they? If
> they are, please mention it in the patch description. If not, please
> move this to a separate patch (or maybe into patch 1).
Yeah should go in the patch that bumps the min version really.
> > +/* Ask for warnings for anything that was marked deprecated in
> > + * the defined version, or before. It is a candidate for rewrite.
> > + */
> > +#define GLIB_VERSION_MIN_REQUIRED GLIB_VERSION_2_40
> > +
> > +/* Ask for warnings if code tries to use function that did not
> > + * exist in the defined version. These risk breaking builds
> > + */
> > +#define GLIB_VERSION_MAX_ALLOWED GLIB_VERSION_2_40
> > +
> > +_Pragma("GCC diagnostic push")
> > +_Pragma("GCC diagnostic ignored \"-Wdeprecated-declarations\"")
>
> Why not "#pragma" ? I think that's a little bit more common?
I tend to always use _Pragma as that offers superset of functionality
of #pramga, because _Pragma can be used in macros.
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 :|
