[libcxx-commits] [PATCH] D119335: [libc++][NFC] Reformat and modernize compressed_pair.h

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone accepted this revision.
Quuxplusone added a comment.

Looks acceptable to me; I'm not sure it buys us anything, but it looks harmless.



================
Comment at: libcxx/include/__memory/compressed_pair.h:68
+  template <class _Up, class = __enable_if_t<!is_same<__compressed_pair_elem, typename decay<_Up>::type>::value>>
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR explicit
+  __compressed_pair_elem(_Up&& __u) : __value_type(std::forward<_Up>(__u)) {}
----------------
ldionne wrote:
> Suggestion: move the `explicit` to the next line, otherwise it's really easy to miss.
+1, `explicit` should come on the same line (and immediately preceding) the ctor's name.
I'd like to see `explicit` added to the ctors on new lines 63, 64, 65, and 74 as well, or know the reason why that would break things. Implicit conversions are the devil, and we shouldn't //need// any non-explicit ctors anywhere on the implementation side of things. (As opposed to on the public-API side, like `std::pair` and `std::vector` and so on, where the Standard mandates many ctors to be non-explicit.)


================
Comment at: libcxx/include/__memory/compressed_pair.h:94-95
 
-    typedef _LIBCPP_NODEBUG __compressed_pair_elem<_T1, 0> _Base1;
-    typedef _LIBCPP_NODEBUG __compressed_pair_elem<_T2, 1> _Base2;
+    using _Base1 = _LIBCPP_NODEBUG __compressed_pair_elem<_T1, 0>;
+    using _Base2 = _LIBCPP_NODEBUG __compressed_pair_elem<_T2, 1>;
 
----------------
If Clang is happy with this, probable yikes. `git grep` tells me that the majority position is pre-`=`:
```
using _Base1 _LIBCPP_NODEBUG = __compressed_pair_elem<_T1, 0>;
```
and I'd be worried that post-`=` meant something different to the compiler.


================
Comment at: libcxx/include/__memory/compressed_pair.h:108
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR
+  __compressed_pair(_U1&& __t1, _U2&& __t2) : _Base1(std::forward<_U1>(__t1)), _Base2(std::forward<_U2>(__t2)) {}
 
----------------
FWIW, I'd prefer to leave this `: ...` on the next line, for clarity and to match line 115 below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119335