diff mbox series

Implement sane variant converting constructor (P0608R3)

Message ID 20190516203026.GA12407@redhat.com
State New
Headers show
Series Implement sane variant converting constructor (P0608R3) | expand

Commit Message

Jonathan Wakely May 16, 2019, 8:30 p.m. UTC
* include/std/variant (__overload_set): Remove.
	(_Arr): New helper.
	(_Build_FUN): New class template to define a single FUN overload,
	with specializations to prevent unwanted conversions, as per P0608R3.
	(_Build_FUNs): New class template to build an overload set of FUN.
	(_FUN_type): New alias template to perform overload resolution.
	(__accepted_type): Use integer_constant base for failure case. Use
	_FUN_type for successful case.
	(variant::__accepted_index): Use _Tp instead of _Tp&&.
	(variant::variant(_Tp&&)): Likewise.
	(variant::operator=(_Tp&&)): Likewise.

Tested powerpc64le-linux, committed to trunk.

Comments

Rainer Orth May 17, 2019, 1:55 p.m. UTC | #1
Hi Jonathan,

> 	* include/std/variant (__overload_set): Remove.
> 	(_Arr): New helper.
> 	(_Build_FUN): New class template to define a single FUN overload,
> 	with specializations to prevent unwanted conversions, as per P0608R3.
> 	(_Build_FUNs): New class template to build an overload set of FUN.
> 	(_FUN_type): New alias template to perform overload resolution.
> 	(__accepted_type): Use integer_constant base for failure case. Use
> 	_FUN_type for successful case.
> 	(variant::__accepted_index): Use _Tp instead of _Tp&&.
> 	(variant::variant(_Tp&&)): Likewise.
> 	(variant::operator=(_Tp&&)): Likewise.
>
> Tested powerpc64le-linux, committed to trunk.
[...]
> diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
> index c6b18d08258..4560f774452 100644
> --- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
> +++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
> @@ -142,6 +142,11 @@ void arbitrary_ctor()
>    static_assert(noexcept(variant<int, DefaultNoexcept>(int{})));
>    static_assert(!noexcept(variant<int, Empty>(Empty{})));
>    static_assert(noexcept(variant<int, DefaultNoexcept>(DefaultNoexcept{})));
> +
> +  // P0608R3 disallow narrowing conversions and boolean conversions
> +  static_assert(!is_constructible_v<variant<int>, long>);
> +  static_assert(!is_constructible_v<variant<bool>, int>);
> +  static_assert(!is_constructible_v<variant<bool>, void*>);

this test (which didn't make it into the ChangeLog, btw.) FAILs on
32-bit targets (seen on i386-pc-solaris2.11, sparc-sun-solaris.11, and
x86_64-pc-linux-gnu -m32):

+FAIL: 20_util/variant/compile.cc (test for excess errors)

Excess errors:
/vol/gcc/src/hg/trunk/local/libstdc++-v3/testsuite/20_util/variant/compile.cc:147: error: static assertion failed

	Rainer
Jonathan Wakely May 17, 2019, 2:11 p.m. UTC | #2
On 17/05/19 15:55 +0200, Rainer Orth wrote:
>Hi Jonathan,
>
>> 	* include/std/variant (__overload_set): Remove.
>> 	(_Arr): New helper.
>> 	(_Build_FUN): New class template to define a single FUN overload,
>> 	with specializations to prevent unwanted conversions, as per P0608R3.
>> 	(_Build_FUNs): New class template to build an overload set of FUN.
>> 	(_FUN_type): New alias template to perform overload resolution.
>> 	(__accepted_type): Use integer_constant base for failure case. Use
>> 	_FUN_type for successful case.
>> 	(variant::__accepted_index): Use _Tp instead of _Tp&&.
>> 	(variant::variant(_Tp&&)): Likewise.
>> 	(variant::operator=(_Tp&&)): Likewise.
>>
>> Tested powerpc64le-linux, committed to trunk.
>[...]
>> diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
>> index c6b18d08258..4560f774452 100644
>> --- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
>> +++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
>> @@ -142,6 +142,11 @@ void arbitrary_ctor()
>>    static_assert(noexcept(variant<int, DefaultNoexcept>(int{})));
>>    static_assert(!noexcept(variant<int, Empty>(Empty{})));
>>    static_assert(noexcept(variant<int, DefaultNoexcept>(DefaultNoexcept{})));
>> +
>> +  // P0608R3 disallow narrowing conversions and boolean conversions
>> +  static_assert(!is_constructible_v<variant<int>, long>);
>> +  static_assert(!is_constructible_v<variant<bool>, int>);
>> +  static_assert(!is_constructible_v<variant<bool>, void*>);
>
>this test (which didn't make it into the ChangeLog, btw.) FAILs on

Drat, not sure how I missed the two test changes out, sorry.

>32-bit targets (seen on i386-pc-solaris2.11, sparc-sun-solaris.11, and
>x86_64-pc-linux-gnu -m32):
>
>+FAIL: 20_util/variant/compile.cc (test for excess errors)
>
>Excess errors:
>/vol/gcc/src/hg/trunk/local/libstdc++-v3/testsuite/20_util/variant/compile.cc:147: error: static assertion failed

I keep forgetting 32-bit exists ;-)

Of course if sizeof(int) == sizeof(long) then it's not a lossy
conversion, and so is allowed.

This patch makes it always a narrowing conversion, irrespective of
target. I'll commit it shortly.
diff mbox series

Patch

commit 0d7f794880e6d7ee6452555e4cd7308f324e402f
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu May 16 20:41:16 2019 +0100

    Implement sane variant converting constructor (P0608R3)
    
            * include/std/variant (__overload_set): Remove.
            (_Arr): New helper.
            (_Build_FUN): New class template to define a single FUN overload,
            with specializations to prevent unwanted conversions, as per P0608R3.
            (_Build_FUNs): New class template to build an overload set of FUN.
            (_FUN_type): New alias template to perform overload resolution.
            (__accepted_type): Use integer_constant base for failure case. Use
            _FUN_type for successful case.
            (variant::__accepted_index): Use _Tp instead of _Tp&&.
            (variant::variant(_Tp&&)): Likewise.
            (variant::operator=(_Tp&&)): Likewise.

diff --git a/libstdc++-v3/include/std/variant b/libstdc++-v3/include/std/variant
index 101b8945943..eec41750da7 100644
--- a/libstdc++-v3/include/std/variant
+++ b/libstdc++-v3/include/std/variant
@@ -161,7 +161,7 @@  namespace __detail
 {
 namespace __variant
 {
-  // Returns the first apparence of _Tp in _Types.
+  // Returns the first appearence of _Tp in _Types.
   // Returns sizeof...(_Types) if _Tp is not in _Types.
   template<typename _Tp, typename... _Types>
     struct __index_of : std::integral_constant<size_t, 0> {};
@@ -727,41 +727,65 @@  namespace __variant
     inline constexpr bool __exactly_once =
       __tuple_count_v<_Tp, tuple<_Types...>> == 1;
 
-  // Takes _Types and create an overloaded _S_fun for each type.
-  // If a type appears more than once in _Types, create only one overload.
-  template<typename... _Types>
-    struct __overload_set
-    { static void _S_fun(); };
+  // Helper used to check for valid conversions that don't involve narrowing.
+  template<typename _Ti> struct _Arr { _Ti _M_x[1]; };
 
-  template<typename _First, typename... _Rest>
-    struct __overload_set<_First, _Rest...> : __overload_set<_Rest...>
+  // Build an imaginary function FUN(Ti) for each alternative type Ti
+  template<size_t _Ind, typename _Tp, typename _Ti,
+	   bool _Ti_is_cv_bool = is_same_v<remove_cv_t<_Ti>, bool>,
+	   typename = void>
+    struct _Build_FUN
     {
-      using __overload_set<_Rest...>::_S_fun;
-      static integral_constant<size_t, sizeof...(_Rest)> _S_fun(_First);
+      // This function means 'using _Build_FUN<I, T, Ti>::_S_fun;' is valid,
+      // but only static functions will be considered in the call below.
+      void _S_fun();
     };
 
-  template<typename... _Rest>
-    struct __overload_set<void, _Rest...> : __overload_set<_Rest...>
+  // ... for which Ti x[] = {std::forward<T>(t)}; is well-formed,
+  template<size_t _Ind, typename _Tp, typename _Ti>
+    struct _Build_FUN<_Ind, _Tp, _Ti, false,
+		      void_t<decltype(_Arr<_Ti>{{std::declval<_Tp>()}})>>
     {
-      using __overload_set<_Rest...>::_S_fun;
+      // This is the FUN function for type _Ti, with index _Ind
+      static integral_constant<size_t, _Ind> _S_fun(_Ti);
     };
 
-  // Helper for variant(_Tp&&) and variant::operator=(_Tp&&).
-  // __accepted_index maps an arbitrary _Tp to an alternative type in _Variant
-  // (or to variant_npos).
+  // ... and if Ti is cv bool, remove_cvref_t<T> is bool.
+  template<size_t _Ind, typename _Tp, typename _Ti>
+    struct _Build_FUN<_Ind, _Tp, _Ti, true,
+		      enable_if_t<is_same_v<__remove_cvref_t<_Tp>, bool>>>
+    {
+      // This is the FUN function for when _Ti is cv bool, with index _Ind
+      static integral_constant<size_t, _Ind> _S_fun(_Ti);
+    };
+
+  template<typename _Tp, typename _Variant,
+	   typename = make_index_sequence<variant_size_v<_Variant>>>
+    struct _Build_FUNs;
+
+  template<typename _Tp, typename... _Ti, size_t... _Ind>
+    struct _Build_FUNs<_Tp, variant<_Ti...>, index_sequence<_Ind...>>
+    : _Build_FUN<_Ind, _Tp, _Ti>...
+    {
+      using _Build_FUN<_Ind, _Tp, _Ti>::_S_fun...;
+    };
+
+  // The index j of the overload FUN(Tj) selected by overload resolution
+  // for FUN(std::forward<_Tp>(t))
+  template<typename _Tp, typename _Variant>
+    using _FUN_type
+      = decltype(_Build_FUNs<_Tp, _Variant>::_S_fun(std::declval<_Tp>()));
+
+  // The index selected for FUN(std::forward<T>(t)), or variant_npos if none.
   template<typename _Tp, typename _Variant, typename = void>
     struct __accepted_index
-    { static constexpr size_t value = variant_npos; };
+    : integral_constant<size_t, variant_npos>
+    { };
 
-  template<typename _Tp, typename... _Types>
-    struct __accepted_index<
-      _Tp, variant<_Types...>,
-      void_t<decltype(__overload_set<_Types...>::_S_fun(std::declval<_Tp>()))>>
-    {
-      static constexpr size_t value = sizeof...(_Types) - 1
-	- decltype(__overload_set<_Types...>::
-		   _S_fun(std::declval<_Tp>()))::value;
-    };
+  template<typename _Tp, typename _Variant>
+    struct __accepted_index<_Tp, _Variant, void_t<_FUN_type<_Tp, _Variant>>>
+    : _FUN_type<_Tp, _Variant>
+    { };
 
   // Returns the raw storage for __v.
   template<typename _Variant>
@@ -1247,8 +1271,8 @@  namespace __variant
 	__exactly_once = __detail::__variant::__exactly_once<_Tp, _Types...>;
 
       template<typename _Tp>
-	static constexpr size_t __accepted_index =
-	  __detail::__variant::__accepted_index<_Tp&&, variant>::value;
+	static constexpr size_t __accepted_index
+	  = __detail::__variant::__accepted_index<_Tp, variant>::value;
 
       template<size_t _Np, typename = enable_if_t<(_Np < sizeof...(_Types))>>
 	using __to_type = variant_alternative_t<_Np, variant>;
@@ -1290,7 +1314,7 @@  namespace __variant
 	constexpr
 	variant(_Tp&& __t)
 	noexcept(is_nothrow_constructible_v<_Tj, _Tp>)
-	: variant(in_place_index<__accepted_index<_Tp&&>>,
+	: variant(in_place_index<__accepted_index<_Tp>>,
 		  std::forward<_Tp>(__t))
 	{ }
 
@@ -1344,7 +1368,7 @@  namespace __variant
 	noexcept(is_nothrow_assignable_v<__accepted_type<_Tp&&>&, _Tp>
 		 && is_nothrow_constructible_v<__accepted_type<_Tp&&>, _Tp>)
 	{
-	  constexpr auto __index = __accepted_index<_Tp&&>;
+	  constexpr auto __index = __accepted_index<_Tp>;
 	  if (index() == __index)
 	    std::get<__index>(*this) = std::forward<_Tp>(__rhs);
 	  else
diff --git a/libstdc++-v3/testsuite/20_util/variant/compile.cc b/libstdc++-v3/testsuite/20_util/variant/compile.cc
index c6b18d08258..4560f774452 100644
--- a/libstdc++-v3/testsuite/20_util/variant/compile.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/compile.cc
@@ -142,6 +142,11 @@  void arbitrary_ctor()
   static_assert(noexcept(variant<int, DefaultNoexcept>(int{})));
   static_assert(!noexcept(variant<int, Empty>(Empty{})));
   static_assert(noexcept(variant<int, DefaultNoexcept>(DefaultNoexcept{})));
+
+  // P0608R3 disallow narrowing conversions and boolean conversions
+  static_assert(!is_constructible_v<variant<int>, long>);
+  static_assert(!is_constructible_v<variant<bool>, int>);
+  static_assert(!is_constructible_v<variant<bool>, void*>);
 }
 
 struct none { none() = delete; };
diff --git a/libstdc++-v3/testsuite/20_util/variant/run.cc b/libstdc++-v3/testsuite/20_util/variant/run.cc
index 0416fbac12a..ac60ccbc13e 100644
--- a/libstdc++-v3/testsuite/20_util/variant/run.cc
+++ b/libstdc++-v3/testsuite/20_util/variant/run.cc
@@ -102,6 +102,32 @@  void arbitrary_ctor()
   variant<int, string> v("a");
   VERIFY(holds_alternative<string>(v));
   VERIFY(get<1>(v) == "a");
+
+  {
+    // P0608R3
+    variant<string, bool> x = "abc";
+    VERIFY(x.index() == 0);
+  }
+
+  {
+    // P0608R3
+    struct U {
+      U(char16_t c) : c(c) { }
+      char16_t c;
+    };
+    variant<char, U> x = u'\u2043';
+    VERIFY(x.index() == 1);
+    VERIFY(std::get<1>(x).c == u'\u2043');
+
+    struct Double {
+      Double(double& d) : d(d) { }
+      double& d;
+    };
+    double d = 3.14;
+    variant<int, Double> y = d;
+    VERIFY(y.index() == 1);
+    VERIFY(std::get<1>(y).d == d);
+  }
 }
 
 struct ThrowingMoveCtorThrowsCopyCtor
@@ -168,6 +194,27 @@  void arbitrary_assign()
 
   VERIFY(holds_alternative<string>(variant<int, string>("a")));
   VERIFY(get<1>(v) == "a");
+
+  {
+    // P0608R3
+    using T1 = variant<float, int>;
+    T1 v1;
+    v1 = 0;
+    VERIFY(v1.index() == 1);
+
+    using T2 = variant<float, long>;
+    T2 v2;
+    v2 = 0;
+    VERIFY(v2.index() == 1);
+
+    struct big_int {
+      big_int(int) { }
+    };
+    using T3 = variant<float, big_int>;
+    T3 v3;
+    v3 = 0;
+    VERIFY(v3.index() == 1);
+  }
 }
 
 void dtor()