[libcxx-commits] [PATCH] D96657: [libcxx] adds common_reference to <type_traits>

Thu Feb 25 18:38:19 PST 2021

cjdb added a comment.

Thanks for your review here, it's really appreciated!

================
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:
----------------
ldionne wrote:
> `_LIBCPP_HAS_NO_CONCEPTS`?
Bleh, I need to merge that patch first. That's going through CI right now, will probably update tomorrow morning (UTC-8).

================
Comment at: libcxx/include/type_traits:2448
+template <class _From, class _To>
+using __copy_cv = typename __copy_cv_<_From>::template __res_type<_To>;
+
----------------
ldionne wrote:
> 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?
> I think it would be a bit easier to follow if you followed the convention of using _t for aliases.

Done.

> Also, this could be defined outside of the block for common_reference since it's a useful utility on its own.

Done.

> You could also add __copy_cvref, and use that to (trivially) implement __xref below.

Isn't __copy_cvref just another spelling of __xref? Or is that your point? :-)

================
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>&>;
----------------
ldionne wrote:
> I don't understand the need for this requires-clause, can you explain?
I had a bug somewhere because I accidentally passed in a reference instead of plain old `T` IIRC. I wanted some guard rails after that.

================
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>
----------------
ldionne wrote:
> 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.
Sure. LMK if you want this for `std::ranges::swap` too (not a part of this review).

================
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>
----------------
ldionne wrote:
> 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?
> 
> Am I right? Is this a bug or am I confused?

This is a bug. Fixed.

> If this is indeed straying from the spec, does it change the result we have in any case?
> If so, is it possible to add a test that would catch this?

I haven't been able to produce a case where `_Ap` and `_Ap&&` disagree.

================
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)
----------------
ldionne wrote:
> The indentation is a bit strange - is that on purpose?
@miscco is the person to answer that. Now that the full passage is there, I've de-indented it a fair bit.

================
Comment at: libcxx/include/type_traits:2537
+template <class _Tp>
+struct _LIBCPP_TEMPLATE_VIS common_reference<_Tp>
+{
----------------
ldionne wrote:
> 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`.
What's this macro for?

================
Comment at: libcxx/include/type_traits:2537
+template <class _Tp>
+struct _LIBCPP_TEMPLATE_VIS common_reference<_Tp>
+{
----------------
cjdb wrote:
> ldionne wrote:
> > 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`.
> What's this macro for?
Removed.

================
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 [...]
----------------
ldionne wrote:
> 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.
This is great, thanks!

================
Comment at: libcxx/include/type_traits:2602
+{};
+
+#endif // _LIBCPP_STD_VER > 17 && defined(__cpp_concepts) && __cpp_concepts >= 201811L
----------------
ldionne wrote:
> 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 { ];
> ```
Do we want `{ }` or `{ using type = __ill_formed; }`? Which is clearer for (a) the user and (b) maintainers?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96657