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

[Jonathan Wakely]

On 02/12/16 19:14 -0800, Tim Shen wrote:
>On Wed, Nov 30, 2016 at 8:27 AM, Jonathan Wakely wrote:
>> 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.
>
>Yay! Also removed the unused #include.
>
>>
>>> -  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"
>
>Change this section of comment to the discussed content.
>
>>
>>> +  // 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".
>
>N/A.
>
>>
>>> +  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!
>
>Already discussed.
>
>>
>>>   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).
>
>Done.
>
>>
>> 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.
>
>Done
>
>>
>>> @@ -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
>
>Done.
>
>>
>>> +      return __lhs._M_equal_to(__rhs,
>>> +
>>> std::make_index_sequence<sizeof...(_Types)>{});
>>
>>
>> Another one that could use index_sequence_for<_Types...>
>
>Done.
>
>>
>>> +      return __lhs._M_less_than(__rhs,
>>> +
>>> std::make_index_sequence<sizeof...(_Types)>{});
>>
>>
>> Same again.
>
>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...>
>
>Done.
>
>>
>>> @@ -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.
>
>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?
>
>Done. I just realized that is_nothrow_callable also handles crazy
>member pointer cases.
>
>Used fold expression instead of __and_ for consistency.
>
>>
>>
>>> @@ -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.
>
>Ah that's an oversight. Moved the static variable out of visit().
>_S_vtable needs to be static, otherwise runtime O(n) assignment will
>happen, where n is the size of _S_vtable.

This looks good - OK for trunk, thanks!