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

[dynamic] Initialization issue #453

Closed
sdebionne opened this issue Mar 16, 2020 · 7 comments · Fixed by #486
Closed

[dynamic] Initialization issue #453

sdebionne opened this issue Mar 16, 2020 · 7 comments · Fixed by #486
Labels
cat/annoyance Not a bug, not a feature, but something that should be improved

Comments

@sdebionne
Copy link
Contributor

Actual behavior

boost::gil::any_image<
  boost::mp11::mp_list<
    boost::gil::gray8_image_t
  >
> img = boost::gil::gray8_image_t();

Fails to compile with

error: conversion from 'boost::gil::gray8_image_t {aka 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> >}' to non-scalar type 'boost::gil::any_image<boost::mp11::mp_list<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> > > >' requested

while

boost::gil::any_image<
  boost::mp11::mp_list<
    boost::gil::gray8_image_t
  >
> img(boost::gil::gray8_image_t());

compiles fine.

Expected behavior

It would be nice if both initializations work the same.

C++ Minimal Working Example

#include <boost/gil.hpp>
#include <boost/gil/extension/dynamic_image/any_image.hpp>
namespace gil = boost::gil;
int main
{
  any_image<boost::mp11::mp_list<gray8_image_t>> img = gray8_image_t();
}

Environment

  • Compiler version: GCC 7.3
  • Build settings: -std=gnu++1z
  • Version (Git ref or <boost/version.hpp>): 1.75
@mloskot mloskot added the cat/annoyance Not a bug, not a feature, but something that should be improved label Mar 18, 2020
@mloskot
Copy link
Member

mloskot commented Mar 18, 2020

@sdebionne yes, it is desired. I have only applied very minimal C++11 modernisation to the dynamic_image extension, it needs more.

By the way, we are likely removing some of apparently dead code from the dynamic_image extension (see #281) and we will also be looking into adding move semantics to image classes (see #438).

@sdebionne
Copy link
Contributor Author

@mloskot I can have a shot at #438 (I think this issue is related) in nobody is working on it.

@mloskot
Copy link
Member

mloskot commented May 6, 2021

@sdebionne I discovered that we might have lost the boost::gil::any_image<boost::mp11::mp_list<...>> completely, what is somewhat related to this issue.

There have been number of changes to any_image and apply_operation recently and earlier, and I might have got lost.
Could you update me on what is currently supported form of specialising any_image?

Here is what I'm testing using Boost 1.76, https://godbolt.org/z/4Ta31Tf3P, and below:

#include <boost/gil.hpp>
#include <boost/gil/extension/dynamic_image/any_image.hpp>
#include <boost/mp11.hpp>
namespace gil = boost::gil;

int main()
{
    // OK
    {
        gil::any_image
        <
            gil::gray8_image_t,
            gil::rgb8_image_t
        > image;
        image.dimensions();
    }
    // not OK
    {
        gil::any_image
        <
            boost::mp11::mp_list
            <
                gil::gray8_image_t,
                gil::rgb8_image_t
            >
        > image;
        image.dimensions();
    }
}

@sdebionne
Copy link
Contributor Author

The first form is the only one supported for any_image and any_image_view. It's indeed a breaking change that was maybe not advertised enough. But I believe the doc is up-to-date.
I'm a bit surprised that I reported this issue with boost::mp11::mp_list<, that probably means it predates the final variant2 refactoring. The use of inheriting constructors b8f48bc should fix #453.

@mloskot
Copy link
Member

mloskot commented May 8, 2021

@sdebionne Thank for the update. Yes, it does predate the variant2.

I will have to catch up the IO tests with those changes.
I missed that because of the still convoluted way of running the IO tests, what tests are run by our CI, what #define-s are set, etc. so it needs clearing to try avoid any conditional running of tests.

@sdebionne
Copy link
Contributor Author

I thought I had updated all the tests, but since I am usually not running the tests locally with IO tests, I might have left some behind... sorry about that. Indeed, it would be great to have the CI catch these oversights.

@mloskot
Copy link
Member

mloskot commented May 10, 2021

@sdebionne No problem, not your fault, but the tests configuration - it should be no brainer w/o any manual tweaks required.

mloskot added a commit to mloskot/gil that referenced this issue May 10, 2021
mloskot added a commit to mloskot/gil that referenced this issue May 10, 2021
mloskot added a commit that referenced this issue May 11, 2021
sdebionne pushed a commit to sdebionne/gil-reformated that referenced this issue May 26, 2021
sdebionne pushed a commit to sdebionne/gil-reformated that referenced this issue Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/annoyance Not a bug, not a feature, but something that should be improved
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants