[libc++] Implement P0608R3 - A sane variant converting constructor

[lichray]

Summary

Prefer user-defined conversions over narrowing conversions and conversions to bool.

References:
http://wg21.link/p0608

[lichray]

Use less traits

[EricWF]

Has this paper been adopted into the standard yet?

include/variant

1097

I see no need to stop using operator() in favor of a static function. Is there a reason?

Also this would have to be __test.

1109

I would like to see a version of this patch that doesn't use preprocessor expansion to specialize __overload multiple times.

Perhaps adding a bool = is_same<remove_cv_t<First>, bool> to __overload and then specializing once on that?

[EricWF]

include/variant

1109

Or perhaps SFINAE-ing the test member instead of specializing __overload.

Something like this maybe?

template <class _Tp, class... _Types>
struct __overload< _Tp, _Types...> : __overload<_Types...>
{
    using __overload<_Types...>::operator();

    template <bool _EnableBool>
    using _DepIsBool = typename __dependent_type<
        enable_if<is_same_v<remove_cv_t<_Tp>, bool> == _EnableBool>, _EnableBool
    >::type;

    template <class _Up, bool _EnableBool = false, class = _DepIsBool<_EnableBool>>
    auto operator()(_Tp, _Up&& __t) const
      -> decltype(_Tp{__t}, __identity<_Tp>());

    template <class _Up, class _Ap = __uncvref_t<_Up>,
        bool _EnableIfBool = true, class = _DepIsBool<_EnableIfBool>>
    auto operator()(bool, _Up&&, int _Ap::*) const
        -> __identity<_Tp>;

    template <class _Up, class _Ap = __uncvref_t<_Up>,
        bool _EnableIfBool = true, class = _DepIsBool<_EnableIfBool>>
    auto operator()(bool, _Up&&) const
        -> enable_if_t<is_same_v<_Ap, bool>, __identity<_Tp>>;
};

[lichray]

In D44865#1048880, @EricWF wrote:

Has this paper been adopted into the standard yet?

No. However, it cleanly passed LEWG for recommending a DR against C++17, so I think we can apply it early, just like we sometimes make changes before filing issues :)

include/variant

1109

If we have concepts we can do so, but __overload is already variadic, leaving no space for sfinae...

[lichray]

Keep changes small

[lichray]

include/variant

1109

Instantiating function templates and sfinae are the slowest operations in meta programming, and I don't want to do this for all alternatives just because of a rare bool.

[EricWF]

include/variant

1109

Hmm. That doesn't seem to quite work..

[EricWF]

include/variant

1109

How would you go about testing this?

[lichray]

include/variant

1109

I fixed the int _Ap::* -> int _Ap::* = 0 part, but it still doesn't work

[...]test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp:190:37: error: no viable conversion from 'bool' to 'std::variant<std::string, bool>' (aka 'variant<basic_string<char, char_traits<char>, allocator<char> >, bool>')
    std::variant<std::string, bool> v = true;
                                    ^   ~~~~

[lichray]

Macro-free

[EricWF]

This LGTM.

However, we should add tests for const bool and volatile bool before committing.

Also I would like @mclow.lists input about applying this DR early since LWG
hasn't commented on it yet.

[lichray]

More tests

[lichray]

Preserve value category in narrowing test.

[lichray]

Refine coding style

[lichray]

Updated implementation for R3

[lichray]

Ping @mclow.lists @EricWF ; the patch still applies, is there any other thing I need to address?

[lichray]

Trivial rebase

[zoecarver]

A few minor things:

• the spacing is different from the rest of libc++ (a lot of it was that way before).
• since this is a paper it should probably be guarded with #if _LIBCPP_STD_VER > 17
• update the status in www.

Also, the standard says explicitly, "A variant is permitted to hold the same type more than once, and to hold differently cv-qualified versions of the same type." But currently, that is not allowed — not an issue with this patch, but something that should be fixed.

include/variant

1110

Is this structure 100% necessary? Couldn't __overload add an overload to take care of bools? Maybe something like this:

template<class _Up>
auto operator()(_Tp, _Up&&) const
  ->  enable_if_t<
          is_same_v<__uncvref_t<_Up>, bool>,
          __identity<_Tp>
      >;

[mclow.lists]

In D44865#1391996, @lichray wrote:

Ping @mclow.lists @EricWF ; the patch still applies, is there any other thing I need to address?

Just a note: P0608R3 was adopted in San Diego.

[lichray]

In D44865#1535823, @zoecarver wrote:

• the spacing is different from the rest of libc++ (a lot of it was that way before).

That's not something I can "fix" in this patch :(

• since this is a paper it should probably be guarded with #if _LIBCPP_STD_VER > 17

Marshall and me agreed that we are going to apply the paper as a "bugfix." We don't want to make the constructor behave differently under different language versions.

• update the status in www.

Done.

include/variant

1110

I'm trying to reduce the compile time cost of SFINAE by putting the bool handling logic in specializations.

[lichray]

Update www

[mclow.lists]

In D44865#1051228, @EricWF wrote:

This LGTM.

Also I would like @mclow.lists input about applying this DR early since LWG
hasn't commented on it yet.

Even though LWG didn't vote it out as a DR; that's what it is, and I think that we would be doing our users a disservice by not providing this in C++17.

[mclow.lists]

The patch looks fine to me, but I think you should consider making a couple of T.fail.cpp tests, and check to make sure you get the "right error".

test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp

136

If you really want to check that these are "ambiguous" , or "no matching operator=", etc, the way to do that is to define a fail.cpp test, and check the error messages.

[EricWF]

include/variant

1128

Do we even support volatile types in variant?

[zoecarver]

include/variant

1128

Yes.

All types in Types shall be (possibly cv-qualified) object types that are not arrays.

[lichray]

Add fail tests

[lichray]

test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp

136

(the messages in static_assert are just notes to people who read this code)

I added additional fail.cpp tests.

[mclow.lists]

This looks good to me. Thanks!

include/variant

bool __move_nothrow() const {
     return this->valueless_by_exception() || __results[this->index()];
   }
 };
 
 template <class... _Types>
 struct __overload;
 
 template <>
-struct __overload<> { void operator()() const; };
+struct __overload<>
+{
+    static void test();
+};
 
 template <class _Tp, class... _Types>
-struct __overload<_Tp, _Types...> : __overload<_Types...> {
-  using __overload<_Types...>::operator();
-  __identity<_Tp> operator()(_Tp) const;
+struct __overload<_Tp, _Types...> : __overload<_Types...>
+{
+    using __overload<_Types...>::test;
+
+    template <class _Up>
+    static auto test(_Tp, _Up&& __t) -> decltype(_Tp{__t}, __identity<_Tp>());
 };
 
+#define _LIBCPP_VARIANT_BOOLEAN_CONVERSION(bool_type)                    \
+    template <class... _Types>                                           \
+    struct __overload<bool_type, _Types...> : __overload<_Types...>      \
+    {                                                                    \
+        using __overload<_Types...>::test;                               \
+                                                                         \
+        template <class _Up, class _Ap = __uncvref_t<_Up>>               \
+        static auto test(bool, _Up&&, int _Ap::* = 0)                    \
+            -> __identity<bool_type>;                                    \
+                                                                         \
+        template <class _Up, class _Ap = __uncvref_t<_Up>>               \
+        static auto test(bool, _Up&&)                                    \
+            -> enable_if_t<is_same_v<_Ap, bool>, __identity<bool_type>>; \
+    }
+
+_LIBCPP_VARIANT_BOOLEAN_CONVERSION(bool);
+_LIBCPP_VARIANT_BOOLEAN_CONVERSION(bool const);
+_LIBCPP_VARIANT_BOOLEAN_CONVERSION(bool volatile);
+_LIBCPP_VARIANT_BOOLEAN_CONVERSION(bool const volatile);
+
+#undef _LIBCPP_VARIANT_BOOLEAN_CONVERSION
+
 template <class _Tp, class... _Types>
-using __best_match_t = typename result_of_t<__overload<_Types...>(_Tp&&)>::type;
+using __best_match_t = typename decltype(
+    __overload<_Types...>::test(declval<_Tp>(), declval<_Tp>()))::type;
 
 } // __variant_detail
 
 template <class... _Types>
 class _LIBCPP_TEMPLATE_VIS variant
     : private __sfinae_ctor_base<
           __all<is_copy_constructible_v<_Types>...>::value,
           __all<is_move_constructible_v<_Types>...>::value>,

test/std/utilities/variant/variant.variant/variant.assign/T.pass.cpp

 
 // template <class T>
 // variant& operator=(T&&) noexcept(see below);
 
 #include <cassert>
 #include <string>
 #include <type_traits>
 #include <variant>
+#include <memory>
 
 #include "test_macros.h"
 #include "variant_test_helpers.hpp"
 
 namespace MetaHelpers {
 
 struct Dummy {
   Dummy() = default;
void test_T_assignment_noexcept() {
   {
     using V = std::variant<Dummy, ThrowsAssignT>;
     static_assert(!std::is_nothrow_assignable<V, int>::value, "");
   }
 }
 
 void test_T_assignment_sfinae() {
   {
-    using V = std::variant<long, unsigned>;
+    using V = std::variant<long, long long>;
     static_assert(!std::is_assignable<V, int>::value, "ambiguous");
   }
   {
     using V = std::variant<std::string, std::string>;
     static_assert(!std::is_assignable<V, const char *>::value, "ambiguous");
   }
   {
     using V = std::variant<std::string, void *>;
     static_assert(!std::is_assignable<V, int>::value, "no matching operator=");
   }
+  {
+    using V = std::variant<std::string, float>;
+    static_assert(!std::is_assignable<V, int>::value, "no matching operator=");
+  }
+  {
+    using V = std::variant<std::unique_ptr<int>, bool>;
+    static_assert(!std::is_assignable<V, std::unique_ptr<char>>::value,
+                  "no explicit bool in operator=");
+  }
 #if !defined(TEST_VARIANT_HAS_NO_REFERENCES)
   {
     using V = std::variant<int, int &&>;
     static_assert(!std::is_assignable<V, int>::value, "ambiguous");
   }
   {
     using V = std::variant<int, const int &>;
     static_assert(!std::is_assignable<V, int>::value, "ambiguous");
void test_T_assignment_basic() {
     std::variant<int, long> v(43l);
     v = 42;
     assert(v.index() == 0);
     assert(std::get<0>(v) == 42);
     v = 43l;
     assert(v.index() == 1);
     assert(std::get<1>(v) == 43);
   }
+  {
+    std::variant<unsigned, long> v;
+    v = 42;
+    assert(v.index() == 1);
+    assert(std::get<1>(v) == 42);
+    v = 43u;
+    assert(v.index() == 0);
+    assert(std::get<0>(v) == 43);
+  }
+  {
+    std::variant<std::string, bool> v = true;
+    v = std::false_type();
+    assert(v.index() == 1);
+    assert(std::get<1>(v) == false);
+    v = "bar";
+    assert(v.index() == 0);
+    assert(std::get<0>(v) == "bar");
+  }
+  {
+    std::variant<bool, std::unique_ptr<int>> v;
+    v = nullptr;
+    assert(v.index() == 1);
+    assert(std::get<1>(v) == nullptr);
+    v = std::true_type();
+    assert(v.index() == 0);
+    assert(std::get<0>(v));
+  }
 #if !defined(TEST_VARIANT_HAS_NO_REFERENCES)
   {
     using V = std::variant<int &, int &&, long>;
     int x = 42;
     V v(43l);
     v = x;
     assert(v.index() == 0);
     assert(&std::get<0>(v) == &x);

test/std/utilities/variant/variant.variant/variant.ctor/T.pass.cpp

 // template <class ...Types> class variant;
 
 // template <class T> constexpr variant(T&&) noexcept(see below);
 
 #include <cassert>
 #include <string>
 #include <type_traits>
 #include <variant>
+#include <memory>
 
 #include "test_convertible.hpp"
 #include "test_macros.h"
 #include "variant_test_helpers.hpp"
 
 struct Dummy {
   Dummy() = default;
 };
void test_T_ctor_noexcept() {
   {
     using V = std::variant<Dummy, ThrowsT>;
     static_assert(!std::is_nothrow_constructible<V, int>::value, "");
   }
 }
 
 void test_T_ctor_sfinae() {
   {
-    using V = std::variant<long, unsigned>;
+    using V = std::variant<long, long long>;
     static_assert(!std::is_constructible<V, int>::value, "ambiguous");
   }
   {
     using V = std::variant<std::string, std::string>;
     static_assert(!std::is_constructible<V, const char *>::value, "ambiguous");
   }
   {
     using V = std::variant<std::string, void *>;
     static_assert(!std::is_constructible<V, int>::value,
                   "no matching constructor");
   }
   {
+    using V = std::variant<std::string, float>;
+    static_assert(!std::is_constructible<V, int>::value,
+                  "no matching constructor");
+  }
+  {
+    using V = std::variant<std::unique_ptr<int>, bool>;
+    static_assert(!std::is_constructible<V, std::unique_ptr<char>>::value,
+                  "no explicit bool in constructor");
+  }
+  {
     using V = std::variant<AnyConstructible, NoConstructible>;
     static_assert(
         !std::is_constructible<V, std::in_place_type_t<NoConstructible>>::value,
         "no matching constructor");
     static_assert(!std::is_constructible<V, std::in_place_index_t<1>>::value,
                   "no matching constructor");
   }
 
void test_T_ctor_basic() {
     static_assert(v.index() == 0, "");
     static_assert(std::get<0>(v) == 42, "");
   }
   {
     constexpr std::variant<int, long> v(42l);
     static_assert(v.index() == 1, "");
     static_assert(std::get<1>(v) == 42, "");
   }
+  {
+    constexpr std::variant<unsigned, long> v(42);
+    static_assert(v.index() == 1, "");
+    static_assert(std::get<1>(v) == 42, "");
+  }
+  {
+    std::variant<std::string, bool const> v = "foo";
+    assert(v.index() == 0);
+    assert(std::get<0>(v) == "foo");
+  }
+  {
+    std::variant<bool volatile, std::unique_ptr<int>> v = nullptr;
+    assert(v.index() == 1);
+    assert(std::get<1>(v) == nullptr);
+  }
 #if !defined(TEST_VARIANT_HAS_NO_REFERENCES)
   {
     using V = std::variant<const int &, int &&, long>;
     static_assert(std::is_convertible<int &, V>::value, "must be implicit");
     int x = 42;
     V v(x);
     assert(v.index() == 0);
     assert(&std::get<0>(v) == &x);