Re: [Patch] Implement std::experimental::variant

[Jonathan Wakely <jwakely at redhat dot com>]

On 15/05/16 20:56 -0700, Tim Shen wrote:
> Hi,
>
> I have implemented
> p0088r2<http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0088r2.html>.

Good to see this is ready for inclusion in lisbtdc++ now, thanks!

> The interface is massive and the sfinae conditions are complicated, so
> I won't be surprised that I made mistakes on interface conformance
> (especially for uses-allocator constructors).

The biggest problem I noticed is that this doesn't work:

#include <experimental/variant>

struct literal {
 constexpr literal() = default;
};

struct nonliteral {
 nonliteral() { }
};

using namespace std::experimental;
   
constexpr variant<literal, nonliteral> v{};
constexpr variant<literal, nonliteral> v1{in_place_type<literal>};
constexpr variant<literal, nonliteral> v2{in_place_index<0>};

Those constructors should be constexpr because in each case the
alternative being constructed is 'literal' which has a constexpr
constructor. I believe this is the hardest part of 'variant' to
implement, so it would be good to know if your current approach is
fundamentally incompatible with this requirement before it's too late
to change it.

> I didn't document the public interfaces, since they may still change;
> I'll document them if it's preferred. Important implementations are
> documented though.

That's fine for now.

> Currently it's in std::experimental::fundamentals_v1. Is there a
> better place to put it in?

The p0088r2 proposal says namespace std, and that's what LEWG advised
in Jacksonville. Let's add it to namespace std on trunk. We have
plenty of time to move it to the experimental namespace (although it
would not be _v1) during GCC 7 stage 1 if the plan changes.

> diff --git a/libstdc++-v3/include/bits/enable_special_members.h b/libstdc++-v3/include/bits/enable_special_members.h
> index 1ac8f38..35c2380 100644
> --- a/libstdc++-v3/include/bits/enable_special_members.h
> +++ b/libstdc++-v3/include/bits/enable_special_members.h
> @@ -36,13 +36,30 @@ namespace std _GLIBCXX_VISIBILITY(default)
> {
> _GLIBCXX_BEGIN_NAMESPACE_VERSION
>
> +  struct _Enable_default_constructor_tag { };

Please give this an explicit default constructor:

struct _Enable_default_constructor_tag
{
 explicit _Enable_default_constructor_tag() = default;
};

This ensures it won't interfere with argument deduction when {} is
used anywhere.

> diff --git a/libstdc++-v3/include/bits/uses_allocator.h b/libstdc++-v3/include/bits/uses_allocator.h
> index b1ff58a..7a331a5 100644
> --- a/libstdc++-v3/include/bits/uses_allocator.h
> +++ b/libstdc++-v3/include/bits/uses_allocator.h
> @@ -109,6 +109,56 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>       return __ret;
>     }
>
> +  template<typename _Tp, typename _Alloc, typename... _Args>
> +    struct __is_uses_allocator_constructible
> +    : __or_<__and_<__not_<uses_allocator<_Tp, _Alloc>>,
> +		   is_constructible<_Tp, _Args...>>,
> +	    __and_<uses_allocator<_Tp, _Alloc>,
> +		   is_constructible<_Tp, allocator_arg_t, _Alloc, _Args...>>,
> +	    __and_<uses_allocator<_Tp, _Alloc>,
> +		   is_constructible<_Tp, _Args..., _Alloc>>> { };

It might be cheaper to spell that like this:

 template<typename _Tp, typename _Alloc, typename... _Args>
   struct __is_uses_allocator_constructible
   : conditional<uses_allocator<_Tp, _Alloc>::value,
       __or_<is_constructible<_Tp, allocator_arg_t, _Alloc, _Args...>,
             is_constructible<_Tp, _Args..., _Alloc>>,
       is_constructible<_Tp, _Args...>>::type { };

That should result in fewer instantiations. Similarly for the nothrow
version.

Although some change are needed to meet the full spec, and the spec
could still change, I think this is OK for trunk, as we have plenty of
time in stage 1 to improve it (or remove it, if necessary).

If you want to go ahead and commit it to trunk please remember to CC
the gcc-patches list. Thanks again for implementing this.