[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 ↗

(On Diff #139704)

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 ↗

(On Diff #139704)

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 ↗

(On Diff #139704)

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 ↗

(On Diff #139704)

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 ↗

(On Diff #139704)

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 ↗

(On Diff #139704)

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

[EricWF]

include/variant

1109 ↗

(On Diff #139704)

How would you go about testing this?

[lichray]

include/variant

1109 ↗

(On Diff #139704)

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 ↗

(On Diff #203733)

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 ↗

(On Diff #203733)

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

130 ↗

(On Diff #203766)

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 ↗

(On Diff #203766)

Do we even support volatile types in variant?

[zoecarver]

include/variant

1128 ↗

(On Diff #203766)

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

130 ↗

(On Diff #203766)

(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!

libcxx/trunk/include/variant

 struct __overload;
 
 template <>
 struct __overload<> { void operator()() const; };
 
 template <class _Tp, class... _Types>
 struct __overload<_Tp, _Types...> : __overload<_Types...> {
   using __overload<_Types...>::operator();
-  __identity<_Tp> operator()(_Tp) const;
+
+  static auto __test(_Tp (&&)[1]) -> __identity<_Tp>;
+
+  template <class _Up>
+  auto operator()(_Tp, _Up&& __t) const
+      -> decltype(__test({ _VSTD::forward<_Up>(__t) }));
+};
+
+template <class _Base, class _Tp>
+struct __overload_bool : _Base {
+  using _Base::operator();
+
+  template <class _Up, class _Ap = __uncvref_t<_Up>>
+  auto operator()(bool, _Up&&) const
+      -> enable_if_t<is_same_v<_Ap, bool>, __identity<_Tp>>;
 };
 
+template <class... _Types>
+struct __overload<bool, _Types...>
+    : __overload_bool<__overload<_Types...>, bool> {};
+template <class... _Types>
+struct __overload<bool const, _Types...>
+    : __overload_bool<__overload<_Types...>, bool const> {};
+template <class... _Types>
+struct __overload<bool volatile, _Types...>
+    : __overload_bool<__overload<_Types...>, bool volatile> {};
+template <class... _Types>
+struct __overload<bool const volatile, _Types...>
+    : __overload_bool<__overload<_Types...>, bool const volatile> {};
+
 template <class _Tp, class... _Types>
-using __best_match_t = typename result_of_t<__overload<_Types...>(_Tp&&)>::type;
+using __best_match_t =
+    typename invoke_result_t<__overload<_Types...>, _Tp, _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>,

libcxx/trunk/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=");
+    struct X {
+      operator void*();
+    };
+    static_assert(!std::is_assignable<V, X>::value,
+                  "no boolean conversion in operator=");
+    static_assert(!std::is_assignable<V, std::false_type>::value,
+                  "no converted to bool in operator=");
+  }
+  {
+    struct X {};
+    struct Y {
+      operator X();
+    };
+    using V = std::variant<X>;
+    static_assert(std::is_assignable<V, Y>::value,
+                  "regression on user-defined conversions 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 = "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);
+  }
+  {
+    std::variant<bool volatile, int> v = 42;
+    v = false;
+    assert(v.index() == 0);
+    assert(!std::get<0>(v));
+    bool lvt = true;
+    v = lvt;
+    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);

libcxx/trunk/test/std/utilities/variant/variant.variant/variant.assign/conv.fail.cpp

+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+// <variant>
+
+// template <class ...Types> class variant;
+
+// template <class T>
+// variant& operator=(T&&) noexcept(see below);
+
+#include <variant>
+#include <string>
+#include <memory>
+
+int main(int, char**)
+{
+  std::variant<int, int> v1;
+  std::variant<long, long long> v2;
+  std::variant<char> v3;
+  v1 = 1; // expected-error {{no viable overloaded '='}}
+  v2 = 1; // expected-error {{no viable overloaded '='}}
+  v3 = 1; // expected-error {{no viable overloaded '='}}
+
+  std::variant<std::string, float> v4;
+  std::variant<std::string, double> v5;
+  std::variant<std::string, bool> v6;
+  v4 = 1; // expected-error {{no viable overloaded '='}}
+  v5 = 1; // expected-error {{no viable overloaded '='}}
+  v6 = 1; // expected-error {{no viable overloaded '='}}
+
+  std::variant<int, bool> v7;
+  std::variant<int, bool const> v8;
+  std::variant<int, bool volatile> v9;
+  v7 = "meow"; // expected-error {{no viable overloaded '='}}
+  v8 = "meow"; // expected-error {{no viable overloaded '='}}
+  v9 = "meow"; // expected-error {{no viable overloaded '='}}
+
+  std::variant<bool> v10;
+  std::variant<bool> v11;
+  std::variant<bool> v12;
+  v10 = std::true_type(); // expected-error {{no viable overloaded '='}}
+  v11 = std::unique_ptr<char>(); // expected-error {{no viable overloaded '='}}
+  v12 = nullptr; // expected-error {{no viable overloaded '='}}
+}

libcxx/trunk/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;
 };
 
 struct ThrowsT {
   ThrowsT(int) noexcept(false) {}
 };
 
 struct NoThrowT {
   NoThrowT(int) noexcept(true) {}
 };
 
 struct AnyConstructible { template <typename T> AnyConstructible(T&&) {} };
 struct NoConstructible { NoConstructible() = delete; };
+template <class T>
+struct RValueConvertibleFrom { RValueConvertibleFrom(T&&) {} };
 
 void test_T_ctor_noexcept() {
   {
     using V = std::variant<Dummy, NoThrowT>;
     static_assert(std::is_nothrow_constructible<V, int>::value, "");
   }
   {
     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");
+    struct X {
+      operator void*();
+    };
+    static_assert(!std::is_constructible<V, X>::value,
+                  "no boolean conversion in constructor");
+    static_assert(!std::is_constructible<V, std::false_type>::value,
+                  "no converted to bool in constructor");
+  }
+  {
+    struct X {};
+    struct Y {
+      operator X();
+    };
+    using V = std::variant<X>;
+    static_assert(std::is_constructible<V, Y>::value,
+                  "regression on user-defined conversions 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);
+  }
+  {
+    std::variant<bool volatile const, int> v = true;
+    assert(v.index() == 0);
+    assert(std::get<0>(v));
+  }
+  {
+    std::variant<RValueConvertibleFrom<int>> v1 = 42;
+    assert(v1.index() == 0);
+
+    int x = 42;
+    std::variant<RValueConvertibleFrom<int>, AnyConstructible> v2 = x;
+    assert(v2.index() == 1);
+  }
 #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);

libcxx/trunk/test/std/utilities/variant/variant.variant/variant.ctor/conv.fail.cpp

+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++98, c++03, c++11, c++14
+
+// <variant>
+
+// template <class ...Types> class variant;
+
+// template <class T> constexpr variant(T&&) noexcept(see below);
+
+#include <variant>
+#include <string>
+#include <memory>
+
+int main(int, char**)
+{
+  std::variant<int, int> v1 = 1; // expected-error {{no viable conversion}}
+  std::variant<long, long long> v2 = 1; // expected-error {{no viable conversion}}
+  std::variant<char> v3 = 1; // expected-error {{no viable conversion}}
+
+  std::variant<std::string, float> v4 = 1; // expected-error {{no viable conversion}}
+  std::variant<std::string, double> v5 = 1; // expected-error {{no viable conversion}}
+  std::variant<std::string, bool> v6 = 1; // expected-error {{no viable conversion}}
+
+  std::variant<int, bool> v7 = "meow"; // expected-error {{no viable conversion}}
+  std::variant<int, bool const> v8 = "meow"; // expected-error {{no viable conversion}}
+  std::variant<int, bool volatile> v9 = "meow"; // expected-error {{no viable conversion}}
+
+  std::variant<bool> v10 = std::true_type(); // expected-error {{no viable conversion}}
+  std::variant<bool> v11 = std::unique_ptr<char>(); // expected-error {{no viable conversion}}
+  std::variant<bool> v12 = nullptr; // expected-error {{no viable conversion}}
+}

libcxx/trunk/www/cxx2a_status.html

-->
   <tr><td><a href="https://wg21.link/P0318R1">P0318R1</a></td><td>LWG</td><td>unwrap_ref_decay and unwrap_reference</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
   <tr><td><a href="https://wg21.link/P0356R5">P0356R5</a></td><td>LWG</td><td>Simplified partial function application</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0357R3">P0357R3</a></td><td>LWG</td><td>reference_wrapper for incomplete types</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
   <tr><td><a href="https://wg21.link/P0482R6">P0482R6</a></td><td>CWG</td><td>char8_t: A type for UTF-8 characters and strings</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0487R1">P0487R1</a></td><td>LWG</td><td>Fixing operator&gt;&gt;(basic_istream&amp;, CharT*) (LWG 2499)</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
   <tr><td><a href="https://wg21.link/P0591R4">P0591R4</a></td><td>LWG</td><td>Utility functions to implement uses-allocator construction</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0595R2">P0595R2</a></td><td>CWG</td><td>P0595R2 std::is_constant_evaluated()</td><td>San Diego</td><td>Complete</td><td>9.0</td></tr>
   <tr><td><a href="https://wg21.link/P0602R4">P0602R4</a></td><td>LWG</td><td>variant and optional should propagate copy/move triviality</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
-  <tr><td><a href="https://wg21.link/P0608R3">P0608R3</a></td><td>LWG</td><td>A sane variant converting constructor</td><td>San Diego</td><td><i> </i></td><td></td></tr>
+  <tr><td><a href="https://wg21.link/P0608R3">P0608R3</a></td><td>LWG</td><td>A sane variant converting constructor</td><td>San Diego</td><td>Complete</td><td>9.0</td></tr>
   <tr><td><a href="https://wg21.link/P0655R1">P0655R1</a></td><td>LWG</td><td>visit&lt;R&gt;: Explicit Return Type for visit</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0771R1">P0771R1</a></td><td>LWG</td><td>std::function move constructor should be noexcept</td><td>San Diego</td><td>Complete</td><td>6.0</td></tr>
   <tr><td><a href="https://wg21.link/P0896R4">P0896R4</a></td><td>LWG</td><td>The One Ranges Proposal</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0899R1">P0899R1</a></td><td>LWG</td><td>P0899R1 - LWG 3016 is not a defect</td><td>San Diego</td><td><i>Nothing to do</i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0919R3">P0919R3</a></td><td>LWG</td><td>Heterogeneous lookup for unordered containers</td><td>San Diego</td><td><i> </i></td><td></td></tr>
   <tr><td><a href="https://wg21.link/P0972R0">P0972R0</a></td><td>LWG</td><td>&lt;chrono&gt; <tt>zero()</tt>, <tt>min()</tt>, and <tt>max()</tt> should be noexcept</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
   <tr><td><a href="https://wg21.link/P1006R1">P1006R1</a></td><td>LWG</td><td>Constexpr in std::pointer_traits</td><td>San Diego</td><td>Complete</td><td>8.0</td></tr>
   <tr><td><a href="https://wg21.link/P1007R3">P1007R3</a></td><td>LWG</td><td><tt>std::assume_aligned</tt></td><td>San Diego</td><td><i> </i></td><td></td></tr>