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

Regression: compilation failure in boost/gil/extension/dynamic_image/any_image.hpp #510

Closed
apolukhin opened this issue Jul 22, 2020 · 7 comments · Fixed by #511
Closed

Comments

@apolukhin
Copy link
Member

Actual behavior

Code that was working fine in Boost. 1.73 now fails to compile:

antoshkka@antishkka-amd:/data/Boost-Cookbook-4880OS/Chapter12/07_gil$ g++-9 main.cpp -I /data/boost/
In file included from /data/boost/boost/gil/extension/dynamic_image/algorithm.hpp:11,
                 from /data/boost/boost/gil/extension/dynamic_image/dynamic_image_all.hpp:11,
                 from /data/boost/boost/gil/extension/toolbox/dynamic_images.hpp:11,
                 from /data/boost/boost/gil/extension/toolbox/toolbox.hpp:13,
                 from /data/boost/boost/gil/io/base.hpp:11,
                 from /data/boost/boost/gil/extension/io/png/tags.hpp:11,
                 from /data/boost/boost/gil/extension/io/png/read.hpp:13,
                 from /data/boost/boost/gil/extension/io/png.hpp:11,
                 from /data/boost/boost/gil/extension/io/png/old.hpp:11,
                 from main.cpp:17:
/data/boost/boost/gil/extension/dynamic_image/any_image.hpp: In substitution of ‘template<class T> using get_view_t = typename T::view_t [with T = boost::mpl::vector3<boost::gil::image<boost::gil::pixel<unsigned char, boost::gil::layout<boost::mp11::mp_list<boost::gil::gray_color_t> > >, false, std::allocator<unsigned char> >, boost::gil::image<boost::gil::pixel<short unsigned int, boost::gil::layout<boost::mp11::mp_list<boost::gil::gray_color_t> > >, false, std::allocator<unsigned char> >, boost::gil::image<boost::gil::pixel<unsigned char, boost::gil::layout<boost::mp11::mp_list<boost::gil::red_t, boost::gil::green_t, boost::gil::blue_t> > >, false, std::allocator<unsigned char> > >]’:
/data/boost/boost/mp11/algorithm.hpp:52:11:   required from ‘struct boost::mp11::detail::mp_transform_impl<boost::gil::detail::get_view_t, boost::gil::any_image<boost::mpl::vector3<boost::gil::image<boost::gil::pixel<unsigned char, boost::gil::layout<boost::mp11::mp_list<boost::gil::gray_color_t>, boost::mp11::mp_list<std::integral_constant<int, 0> > > >, false, std::allocator<unsigned char> >, boost::gil::image<boost::gil::pixel<short unsigned int, boost::gil::layout<boost::mp11::mp_list<boost::gil::gray_color_t>, boost::mp11::mp_list<std::integral_constant<int, 0> > > >, false, std::allocator<unsigned char> >, boost::gil::image<boost::gil::pixel<unsigned char, boost::gil::layout<boost::mp11::mp_list<boost::gil::red_t, boost::gil::green_t, boost::gil::blue_t>, boost::mp11::mp_list<std::integral_constant<int, 0>, std::integral_constant<int, 1>, std::integral_constant<int, 2> > > >, false, std::allocator<unsigned char> > > > >’
/data/boost/boost/gil/extension/dynamic_image/any_image.hpp:93:11:   required from ‘class boost::gil::any_image<boost::mpl::vector3<boost::gil::image<boost::gil::pixel<unsigned char, boost::gil::layout<boost::mp11::mp_list<boost::gil::gray_color_t>, boost::mp11::mp_list<std::integral_constant<int, 0> > > >, false, std::allocator<unsigned char> >, boost::gil::image<boost::gil::pixel<short unsigned int, boost::gil::layout<boost::mp11::mp_list<boost::gil::gray_color_t>, boost::mp11::mp_list<std::integral_constant<int, 0> > > >, false, std::allocator<unsigned char> >, boost::gil::image<boost::gil::pixel<unsigned char, boost::gil::layout<boost::mp11::mp_list<boost::gil::red_t, boost::gil::green_t, boost::gil::blue_t>, boost::mp11::mp_list<std::integral_constant<int, 0>, std::integral_constant<int, 1>, std::integral_constant<int, 2> > > >, false, std::allocator<unsigned char> > > >’
main.cpp:64:38:   required from here
/data/boost/boost/gil/extension/dynamic_image/any_image.hpp:31:7: error: no type named ‘view_t’ in ‘struct boost::mpl::vector3<boost::gil::image<boost::gil::pixel<unsigned char, boost::gil::layout<boost::mp11::mp_list<boost::gil::gray_color_t> > >, false, std::allocator<unsigned char> >, boost::gil::image<boost::gil::pixel<short unsigned int, boost::gil::layout<boost::mp11::mp_list<boost::gil::gray_color_t> > >, false, std::allocator<unsigned char> >, boost::gil::image<boost::gil::pixel<unsigned char, boost::gil::layout<boost::mp11::mp_list<boost::gil::red_t, boost::gil::green_t, boost::gil::blue_t> > >, false, std::allocator<unsigned char> > >’
   31 | using get_view_t = typename T::view_t;
      |       ^~~~~~~~~~

Expected behavior

Code compiles and runs

C++ Minimal Working Example

#include <png.h>

// MinGW workarounds for https://svn.boost.org/trac10/ticket/3908 :
#ifndef png_infopp_NULL
#   define png_infopp_NULL NULL
#endif
#ifndef int_p_NULL
#   define int_p_NULL NULL
#endif

#include <boost/version.hpp>
#if (BOOST_VERSION < 106800)
#   include <boost/gil/gil_all.hpp>
#   include <boost/gil/extension/io/png_dynamic_io.hpp>
#else
#   include <boost/gil.hpp>
#   include <boost/gil/extension/io/png/old.hpp>
#endif
#include <boost/mpl/vector.hpp>

#include <string>

#include <cassert>

struct negate {
    typedef void result_type; // required

    template <class View>
    void operator()(const View& source) const {

// Code formatting is shifted left to fit page width
boost::gil::gil_function_requires<
    boost::gil::ImageViewConcept<View>
>();

typedef typename View::value_type value_type;
typedef typename boost::gil::channel_type<value_type>::type channel_t;

const std::size_t channels = boost::gil::num_channels<View>::value;
const channel_t max_val = (std::numeric_limits<channel_t>::max)();

for (unsigned int y = 0; y < source.height(); ++y) {
    for (unsigned int x = 0; x < source.width(); ++x) {
        for (unsigned int c = 0; c < channels; ++c) {
            source(x, y)[c] = max_val - source(x, y)[c];
        }
    }
}


    }
}; // negate

int main(int argc, char *argv[]) {
    assert(argc == 2);

    typedef boost::mpl::vector3<
            boost::gil::gray8_image_t,
            boost::gil::gray16_image_t,
            boost::gil::rgb8_image_t
    > img_types;

    std::string file_name(argv[1]);
    boost::gil::any_image<img_types> source;
    boost::gil::png_read_image(file_name, source);

    boost::gil::apply_operation(
        view(source),
        negate()
    );

    boost::gil::png_write_view("negate_" + file_name, const_view(source));
}

Taken from here: https://github.com/apolukhin/Boost-Cookbook/blob/second_edition/Chapter12/07_gil/main.cpp

Environment

@sdebionne
Copy link
Contributor

sdebionne commented Jul 24, 2020

It's a breaking change, admittedly not very well documented.

boost::gil::any_image<
    boost::gil::gray8_image_t,
    boost::gil::gray16_image_t,
    boost::gil::rgb8_image_t> source;

should work. No need for a MPL Sequence anymore since we have variadic template in C++11 (required for GIL since 1.68).

@mloskot
Copy link
Member

mloskot commented Jul 24, 2020

Basically, as part of C++11 modernisation started in Boost 1.68, we've removed uses of MPL (replacing them with MP11) in Boost 1.72.
Unfortunately, the changes in Boost 1.72 did not contain complete removal of (indirect) dependency on MPL (e.g. via Boost.Variant, etc.)
In Boost 1.74, thanks to @sdebionne contributions, we have completed the removal of, hopefully, any uses, direct or indirect, of MPL.


I have updated our release notes to make the change clearer f3ba17f


I did not know/or forgot about the Boost Cookbook. I think it is important piece of material and to keep it up to date. I will try to include it in my testing of GIL. Thanks @sdebionne for updating it.

@apolukhin
Copy link
Member Author

It's a breaking change, admittedly not very well documented.

boost::gil::any_image<
    boost::gil::gray8_image_t,
    boost::gil::gray16_image_t,
    boost::gil::rgb8_image_t> source;

should work. No need for a MPL Sequence anymore since we have variadic template in C++11 (required for GIL since 1.68).

I've tried that and now GIL fails to substitute the apply_visitor: https://travis-ci.org/github/apolukhin/Boost-Cookbook/jobs/711455496#L2754

@mloskot
Copy link
Member

mloskot commented Jul 26, 2020

I think, I see the problem:

  1. Although CI jobs passed for PR Use perfect forwarding from apply_operation to visit #491, that change was not verified by any of the C++11 mode builds.
    It, possibly, was only verified by VS builds which are C++14.
    That is due to, still, lack of proper coverages of dynamic_image with tests.

  2. The Visitor argument of apply_operation changed from by-value to &&

  3. Some of Visitor-s are passed by l-value reference, then in C++11 mode this typename Visitor::result_type simply fails and such apply_operation candidate template is simply ignored.

A dumb fix is

 auto apply_operation(Variant1&& arg1, Visitor&& op)
 #if defined(BOOST_NO_CXX14_DECLTYPE_AUTO) || defined(BOOST_NO_CXX11_DECLTYPE_N3276)
-auto apply_operation(Variant1&& arg1, Visitor&& op)
+auto apply_operation(Variant1&& arg1, Visitor op)

or a less dumb

 auto apply_operation(Variant1&& arg1, Visitor&& op)
 #if defined(BOOST_NO_CXX14_DECLTYPE_AUTO) || defined(BOOST_NO_CXX11_DECLTYPE_N3276)
-    -> typename Visitor::result_type
+    -> typename std::remove_reference<Visitor>::type::result_type

@sdebionne What do you think?

@sdebionne
Copy link
Contributor

@mloskot Thank you for investigating this! It passed through the cracks unnoticed on my side too (std > c++11).

  1. dynamic_image needs better test coverage for sure, I'll need to find some time to work on that.

  2. Correct, it sounded sensible to use the same signature than the underlying variant.

  3. I looked quickly at Boost.Variant2 implementation and given the complexity of the Vret<> metafunction, I would be tempted to use it:

 auto apply_operation(Variant1&& arg1, Visitor&& op)
 #if defined(BOOST_NO_CXX14_DECLTYPE_AUTO) || defined(BOOST_NO_CXX11_DECLTYPE_N3276)
-    -> typename Visitor::result_type
+    -> variant2::detail::Vret<Visitor, Variant1>

The problem is the usage of code in the detail namespace. Maybe @pdimov could comment on that?

@pdimov
Copy link
Member

pdimov commented Jul 27, 2020

Use decltype, it's C++11: -> decltype(variant2::visit(std::forward<Visitor>(op), std::forward<Variant1>(arg1)))

@mloskot
Copy link
Member

mloskot commented Jul 27, 2020

I knew there must be a clever way ;) Thanks @pdimov

mloskot pushed a commit that referenced this issue Jul 27, 2020
Fixes regression introduce in #491 when building in C++11
Fixes #510
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants