Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore!: Deprecate any_color_converted_view #660

Conversation

marco-langer
Copy link
Contributor

@marco-langer marco-langer commented May 4, 2022

Description

I suggest to deprecate the function any_color_converted_view from gil/extension/dynamic_image/image_view_factory.hpp, because it has the exact same implementation as the function color_converted_view:

template <typename DstP, typename ...Views>
inline
auto color_converted_view(any_image_view<Views...> const& src)
    -> typename color_converted_view_type<any_image_view<Views...>, DstP>::type
{
    using cc_view_t = typename color_converted_view_type<any_image_view<Views...>, DstP>::type;
    return apply_operation(src, detail::color_converted_view_fn<DstP, cc_view_t>());
}
template <typename DstP, typename ...Views>
inline
auto any_color_converted_view(const any_image_view<Views...>& src)
    -> typename color_converted_view_type<any_image_view<Views...>, DstP>::type
{
    using cc_view_t = typename color_converted_view_type<any_image_view<Views...>, DstP>::type;
    return apply_operation(src, detail::color_converted_view_fn<DstP, cc_view_t>());
}

According to the documentation of any_color_converted_view, the prefix any_ was added to avoid an ambiguous overload resolution for GCC 3.4, which however, we do not have to support anymore with C++11.

Tasklist

  • Ensure all CI builds pass
  • Review and approve

@sdebionne
Copy link
Contributor

Makes sense, thanks! Actually I wonder if we could remove it alltogether...

@striezel
Copy link
Contributor

striezel commented May 4, 2022

Actually I wonder if we could remove it alltogether...

As far as I understand Boost's policy with regards to deprecation there needs to be at least one version / release where stuff that will get removed has to be announced as deprecated. Then the next release can actually remove that stuff.

So it could be something like:

  • Boost 1.79.0: feature X is still present.
  • Boost 1.80.0: announce deprecation
  • Boost 1.81.0: remove feature X

@mloskot
Copy link
Member

mloskot commented May 18, 2022

Boost 1.80.0: announce deprecation

Yes, we need to deprecate it first with a release note similar to this
8b1c2d3

@mloskot mloskot changed the title deprecate any_color_converted_view chore!: Deprecate any_color_converted_view May 18, 2022
@mloskot mloskot added this to the Boost 1.80 milestone May 18, 2022
@mloskot mloskot added the cat/deprecation Deprecate a feature or functionality label May 18, 2022
@striezel
Copy link
Contributor

Yes, we need to deprecate it first with a release note

I've just created a pull request with the corresponding release notes to get things moving: #678.

@mloskot
Copy link
Member

mloskot commented May 25, 2022

@striezel AFAIU, this PR is to be closed in favour of #678 Correct?

@striezel
Copy link
Contributor

Yes, it can be closed.

However, after the release of Boost 1.80 a new PR can (and should) be created that actually removes the then deprecated any_color_converted_view.

@marco-langer
Copy link
Contributor Author

marco-langer commented May 26, 2022

In my PR here I also changed the implementation of the functions in order to remove the invocation of apply_operation(). This will be necessary anyway in order to complete #656 (actually I postponed further work on #656 and waited until this PR gets merged, because both have merge conflicts).

#678 hasn't changed the implementation of the deprecated functions. But I can rebase #656 later on and change the implementation again if #678 gets merged.

@mloskot
Copy link
Member

mloskot commented May 26, 2022

@marco-langer Good point. Thank you very much for your help!


I'm closing this PR and I'm going to merge @striezel 's #678.

@mloskot mloskot closed this May 26, 2022
mloskot pushed a commit that referenced this pull request May 26, 2022
color_converted_view has the same implementation and can be used
as a replacement, providing the same functionality.

This builds on #660 by @marco-langer (Thank you!)

Due to the fact that the switch to C++14 has been announced in #677,
we can now officially use the [[deprecated]] attribute which was standardized
in C++14.

This is initial part of deprecating the `any_color_converted_view` and
as @marco-langer pointed in
#660 (comment)
there are more changes to follow.
@mloskot mloskot added the ext/dynamic_image boost/gil/extension/dynamic_image label May 26, 2022
@marco-langer
Copy link
Contributor Author

marco-langer commented Jun 4, 2022

Since #678 got merged, I noticed that the deprecation warning is triggered with some compilers even if the function is not invoked at all:

#include <boost/gil.hpp>
#include <boost/gil/extension/io/png.hpp>

int main() {}

Compiled with gcc 10.2.1:

In file included from ../../boost/boost/gil/extension/dynamic_image/dynamic_image_all.hpp:14,
                 from ../../boost/boost/gil/extension/toolbox/dynamic_images.hpp:11,
                 from ../../boost/boost/gil/extension/toolbox/toolbox.hpp:13,
                 from ../../boost/boost/gil/io/base.hpp:11,
                 from ../../boost/boost/gil/extension/io/png/tags.hpp:11,
                 from ../../boost/boost/gil/extension/io/png/read.hpp:13,
                 from ../../boost/boost/gil/extension/io/png.hpp:11,
                 from main.cpp:2:
../../boost/boost/gil/extension/dynamic_image/image_view_factory.hpp: In function ‘typename boost::gil::color_converted_view_type<boost::gil::any_image_view<Types ...>, DstP, CC>::type boost::gil::any_color_converted_view(const boost::gil::any_image_view<Types ...>&, CC)’:
../../boost/boost/gil/extension/dynamic_image/image_view_factory.hpp:388:71: warning: ‘typename boost::gil::color_converted_view_type<boost::gil::any_image_view<Types ...>, DstP, CC>::type boost::gil::any_color_converted_view(const boost::gil::any_image_view<Types ...>&, CC)’ is deprecated: Use color_converted_view(const any_image_view<Views...>& src, CC) instead. [-Wdeprecated-declarations]
  388 |     return apply_operation(src, detail::color_converted_view_fn<DstP, cc_view_t>());
      |                                                                       ^~~~~~~~~
../../boost/boost/gil/extension/dynamic_image/image_view_factory.hpp:384:6: note: declared here
  384 | auto any_color_converted_view(const any_image_view<Views...>& src, CC)
      |      ^~~~~~~~~~~~~~~~~~~~~~~~
../../boost/boost/gil/extension/dynamic_image/image_view_factory.hpp: In function ‘typename boost::gil::color_converted_view_type<boost::gil::any_image_view<Types ...>, DstP>::type boost::gil::any_color_converted_view(const boost::gil::any_image_view<Types ...>&)’:
../../boost/boost/gil/extension/dynamic_image/image_view_factory.hpp:402:71: warning: ‘typename boost::gil::color_converted_view_type<boost::gil::any_image_view<Types ...>, DstP>::type boost::gil::any_color_converted_view(const boost::gil::any_image_view<Types ...>&)’ is deprecated: Use color_converted_view(any_image_view<Views...> const& src) instead. [-Wdeprecated-declarations]
  402 |     return apply_operation(src, detail::color_converted_view_fn<DstP, cc_view_t>());
      |                                                                       ^~~~~~~~~
../../boost/boost/gil/extension/dynamic_image/image_view_factory.hpp:398:6: note: declared here
  398 | auto any_color_converted_view(const any_image_view<Views...>& src)
      |      ^~~~~~~~~~~~~~~~~~~~~~~~

This warning is not emitted with clang 11. Currently I think it is a compiler bug in gcc 10.2.1, because we never invoke the deprecated function in the given example. Actually I already noticed this behavior while working on this PR and simply forgot to mention it as we had the discussion whether to merge this PR or #678. This PR does not emit the warning.

I will try to complete #656 ASAP in order to solve this new problem, but also in order to get the apply_operation deprecation shipped with Boost 1.80.

@mloskot
Copy link
Member

mloskot commented Jun 6, 2022

@marco-langer

Currently I think it is a compiler bug in gcc 10.2.1

I think you may be correct: GCC Bug 95302 - function attributed to be deprecated cannot include a typedef/using
It is marked as "Known to fail: 10.2.0"
It looks like the using cc_view_t = needs to be unrolled in places of its use instead.

Thank you!

mloskot pushed a commit that referenced this pull request Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/deprecation Deprecate a feature or functionality ext/dynamic_image boost/gil/extension/dynamic_image
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants