[libcxx-commits] [PATCH] D96657: [libcxx] adds common_reference to <type_traits>
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Feb 25 09:58:39 PST 2021
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
Thanks a lot for working on this (both you and @miscco). I left some comments - some of them are about re-organization and could be seen as being subjective. If you disagree with some of my suggestions, let me know and we can talk.
Just for the sake of curiosity, I did try to re-implement `COMMON-REF` from the spec because I was struggling to get an understanding of what that beast does (and hence give a meaningful review). Here's what I came up with. I am *not* suggesting that we go down that route, I'm just posting it so it doesn't get lost forever since I think it's an interesting take on the problem (using `if constexpr`). I think your solution is superior though.
template <class _Tp>
struct __type { using type = _Tp; };
struct __illformed;
template <class _Tp>
constexpr bool __is_well_formed_v = !is_same_v<_Tp, __illformed>;
// __maybe_t applies an alias to the provided arguments, and returns
// `__illformed` if the alias isn't well-formed with those arguments.
template <template <class ...> class _Fp, class ..._Args>
struct __maybe {
using type = __illformed;
};
template <template <class ...> class _Fp, class ..._Args>
requires requires { typename _Fp<_Args...>; }
struct __maybe<_Fp, _Args...> {
using type = _Fp<_Args...>;
};
template <template <class ...> class _Fp, class ..._Args>
using __maybe_t = typename __maybe<_Fp, _Args...>::type;
template <class _Xp, class _Yp>
struct __cond_res_criterion {
using _Res = __maybe_t<__cond_res, __copy_cv<_Xp, _Yp>&, __copy_cv<_Yp, _Xp>&>;
static constexpr bool value = __is_well_formed_v<_Res> && is_reference_v<_Res>;
};
template <class _Ap, class _Bp, class _Xp, class _Yp>
struct _Cp_criterion;
template <class _Ap, class _Bp, class _Xp, class _Yp>
struct _Dp_criterion;
template <class _Ap, class _Bp>
auto __common_ref_impl() {
using _Xp = remove_reference_t<_Ap>;
using _Yp = remove_reference_t<_Bp>;
// If A and B are both lvalue reference types, COMMON-REF(A, B) is
// COND-RES(COPYCV(X, Y) &, COPYCV(Y, X) &) if that type exists and
// is a reference type.
if constexpr (_And<is_lvalue_reference<_Ap>,
is_lvalue_reference<_Bp>,
__cond_res_criterion<_Xp, _Yp>>::value) {
using _Res = typename __cond_res_criterion<_Xp, _Yp>::_Res;
return __type<_Res>{};
}
// Otherwise, let C be remove_reference_t<COMMON-REF(X&, Y&)>&&.
// If A and B are both rvalue reference types,
// C is well-formed,
// and is_convertible_v<A, C> && is_convertible_v<B, C> is true,
// then COMMON-REF(A, B) is C.
else if constexpr (_And<is_rvalue_reference<_Ap>,
is_rvalue_reference<_Bp>,
_Cp_criterion<_Ap, _Bp, _Xp, _Yp>>::value) {
using _Cp = typename _Cp_criterion<_Ap, _Bp, _Xp, _Yp>::_Cp;
return __type<_Cp>{};
}
// Otherwise, let D be COMMON-REF(const X&, Y&).
// If A is an rvalue reference
// and B is an lvalue reference
// and D is well-formed
// and is_convertible_v<A, D> is true,
// then COMMON-REF(A, B) is D.
else if constexpr (_And<is_rvalue_reference<_Ap>,
is_lvalue_reference<_Bp>,
_Dp_criterion<_Ap, _Bp, _Xp, _Yp>>::value) {
using _Dp = typename _Dp_criterion<_Ap, _Bp, _Xp, _Yp>::_Dp;
return __type<_Dp>{};
}
// Otherwise, if A is an lvalue reference and B is an rvalue reference,
// then COMMON-REF(A, B) is COMMON-REF(B, A).
else if constexpr (is_lvalue_reference_v<_Ap> && is_rvalue_reference_v<_Bp>) {
return __common_ref_impl<_Bp, _Ap>();
}
// Otherwise, COMMON-REF(A, B) is ill-formed.
else {
return __type<__illformed>{};
}
}
template <class _Ap, class _Bp, class _Xp, class _Yp>
struct _Cp_criterion {
using _Cp = remove_reference_t<typename decltype(__common_ref_impl<_Xp&, _Yp&>())::type>&&;
static constexpr bool value = __is_well_formed_v<_Cp> &&
is_convertible_v<_Ap, _Cp> &&
is_convertible_v<_Bp, _Cp>;
};
template <class _Ap, class _Bp, class _Xp, class _Yp>
struct _Dp_criterion {
using _Dp = typename decltype(__common_ref_impl<const _Xp&, _Yp&>())::type;
static constexpr bool value = __is_well_formed_v<_Dp> && is_convertible_v<_Ap, _Dp>;
};
template <class _Ap, class _Bp, class _Res = typename decltype(__common_ref_impl<_Ap, _Bp>())::type>
using __common_ref = enable_if_t<__is_well_formed_v<_Res>, _Res>;
================
Comment at: libcxx/include/type_traits:2415
+// common_reference
+#if _LIBCPP_STD_VER > 17 && defined(__cpp_concepts) && __cpp_concepts >= 201811L
+// Let COND_RES(X, Y) be:
----------------
`_LIBCPP_HAS_NO_CONCEPTS`?
================
Comment at: libcxx/include/type_traits:2448
+template <class _From, class _To>
+using __copy_cv = typename __copy_cv_<_From>::template __res_type<_To>;
+
----------------
I think it would be a bit easier to follow if you followed the convention of using `_t` for aliases.
Also, this could be defined outside of the block for `common_reference` since it's a useful utility on its own. You could also add `__copy_cvref`, and use that to (trivially) implement `__xref` below. What do you think?
================
Comment at: libcxx/include/type_traits:2483
+template<class _Xp, class _Yp>
+requires (!is_reference_v<_Xp> && !is_reference_v<_Yp>)
+using __cv_cond_res = __cond_res<__copy_cv<_Xp, _Yp>&, __copy_cv<_Yp, _Xp>&>;
----------------
I don't understand the need for this requires-clause, can you explain?
================
Comment at: libcxx/include/type_traits:2494
+// Otherwise, let C be remove_reference_t<COMMON-REF(X&, Y&)>&&. If A and B are both
+// rvalue reference types, C is well-formed, and ...
+template <class _Xp, class _Yp>
----------------
Do you think it would be possible to paste the whole wording instead of skipping it with `...`? It doesn't add much text and it makes it easier to follow along (IMO). LMK if you disagree.
================
Comment at: libcxx/include/type_traits:2501-2502
+ requires { typename __common_ref_C<_Xp, _Yp>; } &&
+ is_convertible_v<_Ap, __common_ref_C<_Xp, _Yp>> &&
+ is_convertible_v<_Bp, __common_ref_C<_Xp, _Yp>>
+struct __common_ref_<_Ap&&, _Bp&&, _Xp, _Yp>
----------------
The spec says "If `A` is an rvalue reference and `B` is an rvalue reference`, and then requires that `is_convertible<A, C> && is_convertible<B, C>`. Here, you're taking the reference-ness off of `A` and `B` by matching your partial specialization. So I think you need to do `is_convertible_v<_Ap&&, __common_ref_C<_Xp, _Yp>>`, and similarly for `_Bp`.
1. Am I right? Is this a bug or am I confused?
2. If this is indeed straying from the spec, does it change the result we have in any case?
3. If so, is it possible to add a test that would catch this?
================
Comment at: libcxx/include/type_traits:2515
+requires requires { typename __common_ref_D<_Xp, _Yp>; } &&
+ is_convertible_v<_Ap, __common_ref_D<_Xp, _Yp>>
+struct __common_ref_<_Ap&&, _Bp&, _Xp, _Yp>
----------------
You have the same problem as above here.
================
Comment at: libcxx/include/type_traits:2521
+
+// Otherwise, if A is an lvalue reference and B is an rvalue reference,
+// then COMMON-REF(A, B) is COMMON-REF(B, A)
----------------
The indentation is a bit strange - is that on purpose?
================
Comment at: libcxx/include/type_traits:2528-2530
+// bullet 1 - sizeof...(T) == 0
+template <class...>
+struct _LIBCPP_TEMPLATE_VIS common_reference {};
----------------
I think I would do this instead, as it makes the intent a bit clearer IMO:
```
template <class...>
struct common_reference; // don't define it
// bullet 1 - sizeof...(T) == 0
template <>
struct common_reference<> { };
```
================
Comment at: libcxx/include/type_traits:2537
+template <class _Tp>
+struct _LIBCPP_TEMPLATE_VIS common_reference<_Tp>
+{
----------------
General note: I don't think we actually need to use `_LIBCPP_TEMPLATE_VIS` here. I know we do in other places, but I don't think they are necessary anywhere in `type_traits`.
================
Comment at: libcxx/include/type_traits:2544-2582
+// sub-bullet 4 & 5 - [...] if common_type_t<T1, T2> is well-formed [...]
+// - Otherwise, there shall be no member type.
+template <class _Tp, class _Up>
+struct __common_reference4 : common_type<_Tp, _Up>
+{};
+
+// sub-bullet 3 - [...] if COND_RES(T1, T2) is well-formed [...]
----------------
Another way to string this would be:
```
// bullet 3 - sizeof...(T) == 2
template <class _Tp, class _Up> struct __common_reference_sub_bullet3;
template <class _Tp, class _Up> struct __common_reference_sub_bullet2 : __common_reference_sub_bullet3<_Tp, _Up> { };
template <class _Tp, class _Up> struct __common_reference_sub_bullet1 : __common_reference_sub_bullet2<_Tp, _Up> { };
// sub-bullet 1 - If T1 and T2 are reference types and COMMON_REF(T1, T2) is well-formed [...]
template <class _Tp, class _Up>
requires is_reference_v<_Tp> && is_reference_v<_Up> &&
requires { typename __common_ref<_Tp, _Up>; }
struct __common_reference_sub_bullet1<_Tp, _Up>
{
using type = __common_ref<_Tp, _Up>;
};
... do other sub bullets in order ...
```
I think this makes it a bit easier to follow since it highlights that we're basically reimplementing short-circuiting ifs using templates (by making the chain between the various bullets obvious up there). Re-organizing the code this way also allows you to follow the bullets in the order they are in the text instead of in reverse order.
================
Comment at: libcxx/include/type_traits:2602
+{};
+
+#endif // _LIBCPP_STD_VER > 17 && defined(__cpp_concepts) && __cpp_concepts >= 201811L
----------------
And here you can add the last sub-bullet (http://eel.is/c++draft/meta.trans.other#6.4.2) by defining the base template to be empty:
```
template <typename ...>
struct common_reference { ];
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D96657/new/
https://reviews.llvm.org/D96657
More information about the libcxx-commits
mailing list