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 after 1.62: variant<recursive_wrapper<X>, ...> is not constructible from X when X is a variant #100

Open
Kojoley opened this issue Aug 8, 2022 · 3 comments

Comments

@Kojoley
Copy link
Contributor

Kojoley commented Aug 8, 2022

Regressed in 9272805

#include <boost/variant.hpp>

struct Nil {};

typedef boost::variant<
                Nil
              , double
            >
        Atom;

typedef boost::variant<
                Nil
#if 1
                  , boost::recursive_wrapper<Atom>
#else
                  , Atom
#endif
                >
        Base;

int main()
{
    Atom atom;
    Base base = atom;
}

Removing these specializations fixes the compilation

template <class T, class U> struct is_constructible<recursive_wrapper<T>, U > : boost::false_type{};
template <class T, class U> struct is_constructible<recursive_wrapper<T>, const U > : boost::false_type{};
template <class T, class U> struct is_constructible<recursive_wrapper<T>, U& > : boost::false_type{};
template <class T, class U> struct is_constructible<recursive_wrapper<T>, const U& > : boost::false_type{};
template <class T, class U> struct is_constructible<recursive_wrapper<T>, recursive_wrapper<U> > : boost::false_type{};
template <class T, class U> struct is_constructible<recursive_wrapper<T>, const recursive_wrapper<U> > : boost::false_type{};
template <class T, class U> struct is_constructible<recursive_wrapper<T>, recursive_wrapper<U>& > : boost::false_type{};
template <class T, class U> struct is_constructible<recursive_wrapper<T>, const recursive_wrapper<U>& > : boost::false_type{};

apolukhin added a commit that referenced this issue Sep 1, 2022
@apolukhin
Copy link
Member

Compile time error is actually a very good outcome!

recursive_wrapper is inefficient, it should not be used in this case.

In cases when recursive_wrapper is used for actual recursion the is_constructible trait specializations are required. Without the specializations the trait is called on incomplete type and either a wrong trait answer is memorized by the compiler or the static assert from within the type trait is raised.

Anyway, I find explicitness in those variant1<>variant2 conversions a very good feature. In your example, should the variant hold Nil as the 0th-Nil or as a part of Atom? I'm quite sure that different developers would answer differently, which makes that conversion error prone and poorly maintainable.

Fell free to reopen this ticket, if there are some real world examples where the old behavior really required.

@Kojoley
Copy link
Contributor Author

Kojoley commented Sep 1, 2022

recursive_wrapper is inefficient, it should not be used in this case.

I think you are missing the other side here. Let's say Atom is a huge type, storing it directly in Base blows it size substantially, and you have a large container of Bases where Atom is rarely presented -- I see in such a case utilizing recursive_wrapper to minimize Base layout is justifiable.

I'm quite sure that different developers would answer differently, which makes that conversion error prone and poorly maintainable.

The documentation says:

Because variant provides special support for recursive_wrapper, clients may treat the resultant variant as though the wrapper were not present.

The thing is that the previously valid code is broken without any notice and it had not been documented to the present day.

Fell free to reopen this ticket, if there are some real world examples where the old behavior really required.

I don't care that much of this issue, but it blocks migration from old boost versions when there is really no bugs in the user code, and no way to at least temporary turn this 'diagnostic' off. See https://stackoverflow.com/questions/73263043/updating-boostvariant-from-boost-1-60-to-boost-1-79

@apolukhin
Copy link
Member

Yor're right, at least the docs should be changed

@apolukhin apolukhin reopened this Sep 2, 2022
eido79 added a commit to eido79/variant that referenced this issue Nov 11, 2022
Use an overload set to detect if a type can be used in a
converting constructor or assignment, in a similar way as
std::variant.

Also fixes the "regression" mentioned in boostorg#100.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants