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

[Tim Shen]

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.


-- 
Regards,
Tim Shen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: a2.diff
Type: text/x-patch
Size: 10744 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20161203/4faf7cc6/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: b2.diff
Type: text/x-patch
Size: 28893 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20161203/4faf7cc6/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: c2.diff
Type: text/x-patch
Size: 10760 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20161203/4faf7cc6/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: d2.diff
Type: text/x-patch
Size: 20120 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20161203/4faf7cc6/attachment-0003.bin>