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

[Christopher Di Bella via Phabricator via libcxx-commits]

cjdb added a comment.

In D96657#2597384 <https://reviews.llvm.org/D96657#2597384>, @ldionne wrote:

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

Here are the unexpected failures I get when running `ninja check-cxx` locally.

  Failed Tests (3):
    libc++ :: libcxx/gdb/gdb_pretty_printer_test.sh.cpp
    libc++ :: libcxx/modules/stds_include.sh.cpp
    libc++ :: std/iterators/iterator.primitives/iterator.traits/empty.fail.cpp

Could these be impacting CI failure? I can't see how `common_reference` would interact with any of these to the point of failure.



================
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:
> 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>;
> };
> ```
I'm not convinced that's any different to what's currently present, except that your `__xref` is its own type, whereas mine's just an alias for `__copy_cvref`.


================
Comment at: libcxx/include/type_traits:2537
+template <class _Tp>
+struct _LIBCPP_TEMPLATE_VIS common_reference<_Tp>
+{
----------------
ldionne wrote:
> 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.
Thanks, this is good to know :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96657