[PATCH] D27565: [libcxx] Fix __compressed_pair so it doesn't copy the argument multiple times, and add constexpr.

Agustín Bergé via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 4 18:38:39 PDT 2017


K-ballo added a comment.

I don't see any potential layout change, other than the case in which both `T1` and `T2` are empty (which I understand doesn't happen within libc++). As far as I know, the rest of the cases should result in the same layout regardless of whether EBO actually applies or not.

Some notes inline.



================
Comment at: include/memory:2058
 
-template <class _T1, class _T2, bool = is_same<typename remove_cv<_T1>::type,
-                                                     typename remove_cv<_T2>::type>::value,
-                                bool = is_empty<_T1>::value
-                                       && !__libcpp_is_final<_T1>::value,
-                                bool = is_empty<_T2>::value
-                                       && !__libcpp_is_final<_T2>::value
-         >
-struct __libcpp_compressed_pair_switch;
-
-template <class _T1, class _T2, bool IsSame>
-struct __libcpp_compressed_pair_switch<_T1, _T2, IsSame, false, false> {enum {value = 0};};
-
-template <class _T1, class _T2, bool IsSame>
-struct __libcpp_compressed_pair_switch<_T1, _T2, IsSame, true, false>  {enum {value = 1};};
-
-template <class _T1, class _T2, bool IsSame>
-struct __libcpp_compressed_pair_switch<_T1, _T2, IsSame, false, true>  {enum {value = 2};};
-
-template <class _T1, class _T2>
-struct __libcpp_compressed_pair_switch<_T1, _T2, false, true, true>    {enum {value = 3};};
-
-template <class _T1, class _T2>
-struct __libcpp_compressed_pair_switch<_T1, _T2, true, true, true>     {enum {value = 1};};
-
-template <class _T1, class _T2, unsigned = __libcpp_compressed_pair_switch<_T1, _T2>::value>
-class __libcpp_compressed_pair_imp;
+template <class _Tp, int _Idx, bool _IsSmall = is_empty<_Tp>::value
+                                       && !__libcpp_is_final<_Tp>::value>
----------------
Should be `_IsEmpty`


================
Comment at: include/memory:2063
+  typedef typename remove_reference<_Tp>::type& reference;
+  typedef const typename remove_reference<_Tp>::type& const_reference;
 
----------------
If `_Tp` can actually be a reference type, turning it into reference to const here doesn't seem right


================
Comment at: include/memory:2110
+  _LIBCPP_CONSTEXPR explicit __compressed_pair_elem(_Up&& __u)
+      : __value_(_VSTD::forward<_Up>(__u)) {};
 
----------------
This use of `__value_` as a type is overly smart, consider just using `_Tp` for clarity


https://reviews.llvm.org/D27565





More information about the cfe-commits mailing list