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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

This basically LGTM, except for a few comments and the failing CI.



================
Comment at: libcxx/include/type_traits:2615
+
+// bullet 4 - Otherwise, if `common_­type_­t<T1, T2>` is well-formed, then the member typedef type
+// denotes that type.
----------------
This comment looks wrong to me. I think this should be http://eel.is/c++draft/meta.trans.other#6.4.1 instead?


================
Comment at: libcxx/include/type_traits:2448
+template <class _From, class _To>
+using __copy_cv = typename __copy_cv_<_From>::template __res_type<_To>;
+
----------------
cjdb wrote:
> 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? :-)
Sorry, that wasn't clear, but what I meant was:

```
template<class _From, class _To>
struct __copy_cvref { using type = ...; };

template<class _From, class _To>
using __copy_cvref_t = typename __copy_cvref<_From, _To>::type;


// ...
// [Note: `XREF(A)` is `__xref<A>::template __res_type`]
template <class _Tp>
struct __xref {
  template <class _Up>
  using __res_type = __copy_cvref_t<_Tp, _Up>;
};
```


================
Comment at: libcxx/include/type_traits:2537
+template <class _Tp>
+struct _LIBCPP_TEMPLATE_VIS common_reference<_Tp>
+{
----------------
cjdb wrote:
> 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.
The macro expands to `__attribute__((__visibility__("default")))`, which basically makes sure that the RTTI and member functions of the class have default visibility. That shouldn't be necessary for type traits unless I'm missing something.


================
Comment at: libcxx/include/type_traits:2602
+{};
+
+#endif // _LIBCPP_STD_VER > 17 && defined(__cpp_concepts) && __cpp_concepts >= 201811L
----------------
cjdb wrote:
> 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?
I think it definitely *needs* to be `{ }` (i.e. no typedef `type` is provided), otherwise we're not conforming? The way you have it right now seems right to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96657