[libcxx-commits] [PATCH] D58019: Add is_nothrow_convertible (P0758R1)

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver marked 3 inline comments as done.
zoecarver added a comment.

Sounds good, that is probably the better call.



================
Comment at: include/type_traits:1558
+    __or_<
+        is_void<_Fm>, is_function<_To>, is_array<_To>
+    >::value
----------------
ldionne wrote:
> zoecarver wrote:
> > zoecarver wrote:
> > > ldionne wrote:
> > > > I don't think this is useful anymore, as this should be handled by `std::is_convertible`. IOW, I think only the test for `noexcept`-ness of the conversion needs to exist, not for convertibility.
> > > Good catch, you're completely right. In fact, we could probably simplify this a lot and just have it use `is_nothrow_constructible<_Fm, _To>`. I am a bit hesitant to do that because it isn't what `is_nothrow_constructible`'s intended use is, what do you think?
> > Actually correction: `void` will not work in the below `__test_noexcept` method so it has to have its own overload. But I can get rid of the `is_array`/`is_function` checks (because those will be caught by `std::is_convertible`).
> Makes sense, see my suggestion below. I don't think we can use `is_nothrow_constructible`, since the check for non-reference types is:
> ```
> integral_constant<bool, noexcept(_Tp(declval<_Args>()...))>
> ```
> 
> That's not checking whether the conversion operator is `noexcept`, only a constructor.
> 
Fair point, didn't think about that. 


================
Comment at: include/type_traits:1586
+template <typename _Fm, typename _To>
+struct is_nothrow_convertible : __and_<is_convertible<_Fm, _To>,
+    __is_nothrow_convertible_helper<_Fm, _To>>::type
----------------
ldionne wrote:
> How about this:
> 
> ```
> __or<
>   __and_<is_void<_Fm>, is_void<_To>>,
>   __and<is_convertible<_Fm, _To>, __is_nothrow_convertible_helper<_Fm, _To>>
> >
> ```
> 
> And then `__is_nothrow_convertible_helper` doesn't have to care about `void`.
Thanks @ldionne, that is great. After all the tests pass I will upload the new patch. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58019/new/

https://reviews.llvm.org/D58019