[Patches] Add variant constexpr support for visit, comparisons and get

[Jonathan Wakely]

On 26/11/16 21:38 -0800, Tim Shen wrote:
>This 4-patch series contains the following in order:
>
>a.diff: Remove uses-allocator ctors. They are going away, and removing
>it reduces the maintenance burden from now on.

Yay! less code.

>b.diff: Add constexpr support for get<> and comparisons. This patch
>also involves small refactoring of _Variant_storage.
>
>c.diff: Fix some libc++ test failures.
>
>d.diff: Add constexpr support for visit. This patch also removes
>__storage, __get_alternative, and __reserved_type_map, since we don't
>need to support reference/void types for now.
>
>The underlying design doesn't change - we still use the vtable
>approach to achieve O(1) runtime cost even under -O0.

Great stuff.



>            * include/std/variant: Implement constexpr comparison and get<>.
>            * testsuite/20_util/variant/compile.cc: Tests.
>
>diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
>index 2d9303a..a913074 100644
>--- a/libstdc++-v3/include/std/variant
>+++ b/libstdc++-v3/include/std/variant
>@@ -154,31 +154,63 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>   template<typename _Alternative>
>     using __storage = _Alternative;
> 
>-  template<typename _Type, bool __is_literal = std::is_literal_type_v<_Type>>
>+  // _Uninitialized nullify the destructor calls.

This wording confused me slightly. How about:

  "_Uninitialized makes destructors trivial"

>+  // This is necessary, since we define _Variadic_union as a recursive union,
>+  // and we don't want to inspect the union members one by one in its dtor,
>+  // it's slow.

Please change "it's slow" to "that's slow".

>+  template<typename _Type, bool = std::is_literal_type_v<_Type>>
>     struct _Uninitialized;

I'm still unsure that is_literal_type is the right trait here. If it's
definitely right then we should probably *not* deprecate it in C++17!

>   template<typename _Type>
>     struct _Uninitialized<_Type, false>
>     {
>-      constexpr _Uninitialized() = default;
>-
>       template<typename... _Args>
>       constexpr _Uninitialized(in_place_index_t<0>, _Args&&... __args)
>       { ::new (&_M_storage) _Type(std::forward<_Args>(__args)...); }
> 
>+      const _Type& _M_get() const &
>+      {
>+	return *static_cast<const _Type*>(
>+	    static_cast<const void*>(&_M_storage));
>+      }
>+
>+      _Type& _M_get() &
>+      { return *static_cast<_Type*>(static_cast<void*>(&_M_storage)); }
>+
>+      const _Type&& _M_get() const &&
>+      {
>+	return std::move(*static_cast<const _Type*>(
>+	    static_cast<const void*>(&_M_storage)));
>+      }
>+
>+      _Type&& _M_get() &&
>+      {
>+	return std::move(*static_cast<_Type*>(static_cast<void*>(&_M_storage)));
>+      }
>+
>       typename std::aligned_storage<sizeof(_Type), alignof(_Type)>::type
> 	  _M_storage;

I think this could use __aligned_membuf, which would reduce the
alignment requirements for some types (e.g. long long on x86-32).

That would also mean you get the _M_ptr() member so don't need all the
casts.

>+      ~_Variant_storage()
>+      { _M_destroy_impl(std::make_index_sequence<sizeof...(_Types)>{}); }

You can use index_sequence_for<_Types...> here.

>@@ -598,9 +645,9 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 	_S_apply_all_alts(_Array_type& __vtable, index_sequence<__indices...>)
> 	{ (_S_apply_single_alt<__indices>(__vtable._M_arr[__indices]), ...); }
> 
>-      template<size_t __index>
>+      template<size_t __index, typename T>

This needs to be _Tp not T

>+      return __lhs._M_equal_to(__rhs,
>+			       std::make_index_sequence<sizeof...(_Types)>{});

Another one that could use index_sequence_for<_Types...>

>+      return __lhs._M_less_than(__rhs,
>+				std::make_index_sequence<sizeof...(_Types)>{});

Same again.


>            * include/bits/enable_special_members.h: Make
>            _Enable_default_constructor constexpr.
>            * include/std/variant (variant::emplace, variant::swap, std::swap,
>            std::hash): Sfinae on emplace and std::swap; handle __poison_hash bases
>            of duplicated types.
>
>diff --git a/libstdc++-v3/include/bits/enable_special_members.h b/libstdc++-v3/include/bits/enable_special_members.h
>index 07c6c99..4f4477b 100644
>--- a/libstdc++-v3/include/bits/enable_special_members.h
>+++ b/libstdc++-v3/include/bits/enable_special_members.h
>@@ -118,7 +118,8 @@ template<typename _Tag>
>     operator=(_Enable_default_constructor&&) noexcept = default;
> 
>     // Can be used in other ctors.
>-    explicit _Enable_default_constructor(_Enable_default_constructor_tag) { }
>+    constexpr explicit
>+    _Enable_default_constructor(_Enable_default_constructor_tag) { }
>   };
> 
>+      void _M_reset()
>+      {
>+	_M_reset_impl(std::make_index_sequence<sizeof...(_Types)>{});
>+	_M_index = variant_npos;
>+      }
>+
>       ~_Variant_storage()
>-      { _M_destroy_impl(std::make_index_sequence<sizeof...(_Types)>{}); }
>+      { _M_reset(); }

These can also use index_sequence_for<_Types...>

>@@ -1253,14 +1285,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
> 
>   template<typename... _Types>
>     struct hash<variant<_Types...>>
>-    : private __poison_hash<remove_const_t<_Types>>...
>+    : private __detail::__variant::_Variant_hash_base<
>+	variant<_Types...>, std::make_index_sequence<sizeof...(_Types)>>

And again.

>     {
>       using result_type = size_t;
>       using argument_type = variant<_Types...>;
> 
>       size_t
>       operator()(const variant<_Types...>& __t) const
>-      noexcept((... && noexcept(hash<decay_t<_Types>>{}(std::declval<_Types>()))))
>+      noexcept((noexcept(hash<decay_t<_Types>>{}(std::declval<_Types>()))
>+		&& ...))

This could be
__and_<is_nothrow_callable<hash<decay_t<_Types>>(_Types)>...>
but I'm not sure it would be an improvement. The is_callable check is
expensive, but maybe we need it anyway to correctly disable this
function if the hash specialization should be posisoned?


>@@ -1270,17 +1239,20 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
>     }
> 
>   template<typename _Visitor, typename... _Variants>
>-    decltype(auto)
>+    constexpr decltype(auto)
>     visit(_Visitor&& __visitor, _Variants&&... __variants)
>     {
>+      if ((__variants.valueless_by_exception() || ...))
>+	__throw_bad_variant_access("Unexpected index");
>+
>       using _Result_type =
> 	decltype(std::forward<_Visitor>(__visitor)(get<0>(__variants)...));
>-      static constexpr auto _S_vtable =
>+      constexpr auto _S_vtable =

If this isn't static now it could be called simply __vtable, the _S_
prefix is misleading. How many of these _S_vtable variables actually
need to be static? If they're all trivial types and constexpr then it
probably doesn't matter either way, there shouldn't be any difference.

> 	__detail::__variant::__gen_vtable<
> 	  _Result_type, _Visitor&&, _Variants&&...>::_S_apply();
>       auto __func_ptr = _S_vtable._M_access(__variants.index()...);
>       return (*__func_ptr)(std::forward<_Visitor>(__visitor),
>-			   __detail::__variant::__get_storage(__variants)...);
>+			   std::forward<_Variants>(__variants)...);
>     }