[libcxx-commits] [PATCH] D114074: [libc++] Implement P1272R4 (std::byteswap)

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__bit/byteswap.h:25
+
+template <integral _Tp>
+_LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) noexcept {
----------------
Mordante wrote:
> I still think it would be good to constrain the template on the maximum supported `sizeof(_Tp)`. I prefer a compile-time error over run-time bug or error.
Constraints are evil because they cause it to just quietly drop out of overload resolution. I don't think WG21 knows what it's doing these days by adding constraints on everything. At first it seems neato that `using std::byteswap; byteswap(N::Widget())` will quietly choose ADL `N::byteswap` instead of being ambiguous — but really, are we //encouraging// people to call `byteswap` by ADL? I don't think so.
I think it'd be fine to add a static_assert, though, like
```
static_assert(sizeof(_Tp) == 1 || sizeof(_Tp) == 2 || sizeof(_Tp) == 4 || sizeof(_Tp) == 8 || sizeof(_Tp) == 16);
```
Could even refactor this function to use a ladder of `if constexpr ... else if constexpr ...` and then put `static_assert(sizeof(_Tp) == 0, "byteswap is unimplemented for integral types of this size");` in the final `else` branch.


================
Comment at: libcxx/include/__bit/byteswap.h:36
+    return __builtin_bswap64(__val);
+#  ifndef _LIBCPP_HAS_NO_INT128
+  case 16:
----------------
Nit: This indentation is bizarre. I expect clang-format is to blame.  `#ifndef` with no space, plz. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114074