[PATCH] D44865: [libc++] Implement P0608R1 - A sane variant converting constructor

[Zhihao Yuan via Phabricator via cfe-commits]

lichray added inline comments.


================
Comment at: include/variant:1109
 
+#define _LIBCPP_VARIANT_BOOLEAN_CONVERSION(bool_type)                    \
+    template <class... _Types>                                           \
----------------
EricWF wrote:
> lichray wrote:
> > EricWF wrote:
> > > lichray wrote:
> > > > EricWF wrote:
> > > > > EricWF wrote:
> > > > > > 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?
> > > > > 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>>;
> > > > > };
> > > > > 
> > > > > ```
> > > > If we have concepts we can do so, but `__overload` is already variadic, leaving no space for sfinae...
> > > Hmm. That doesn't seem to quite work..
> > 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.
> How would you go about testing this?
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;
                                    ^   ~~~~
```


Repository:
  rCXX libc++

https://reviews.llvm.org/D44865