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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM, I looked at everything and this is indeed NFC. I don't understand why this is failing in C++03 mode, either.

Ship it but please consider my comments.



================
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)) {}
----------------
Suggestion: move the `explicit` to the next line, otherwise it's really easy to miss.


================
Comment at: libcxx/include/__memory/compressed_pair.h:119
 
-  _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11 typename _Base1::reference first() _NOEXCEPT {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11 typename _Base1::reference first() _NOEXCEPT {
     return static_cast<_Base1&>(*this).__get();
----------------
It's more customary to leave the attributes on the line above, like so:

```
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
typename _Base1::reference first() _NOEXCEPT { ... }
```

Otherwise, it's difficult to parse the function declaration itself because it's too obscured by these annotations. I think it's fine to leave the constructors above like `_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __compressed_pair_elem() = default;` as-is since it allows aligning them and their body is empty anyway, so that's more readable.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119335