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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__bit/byteswap.h:25
+template <integral _Tp>
+_LIBCPP_HIDE_FROM_ABI constexpr _Tp byteswap(_Tp __val) noexcept {
+  switch (sizeof(_Tp)) {
----------------
Quuxplusone wrote:
> Mordante wrote:
> > We conditionally support `__uint128_t`. This type won't work with the current implementation. I would prefer to support that type, or constrain function where `sizeof(_Tp) <= 8`. The availability is guarded by `_LIBCPP_HAS_NO_INT128`.
> > 
> > Note that GCC already has a `__builtin_bswap128`, so maybe it's interesting to also add this to clang. I wouldn't mind looking into that myself if you don't want to.
> IMO:
> - Definitely add `_LIBCPP_UNREACHABLE();` at the end of this function.
> - If both Clang and GCC support `__builtin_bswap128` then we should just do that for `case 16`.
> - Otherwise, we should `case 16: return _Tp(byteswap(uint64_t(__val))) | (_Tp(byteswap(uint64_t(__val >> 64))) << 64); // TODO: __builtin_bswap128 when Clang supports it`.
Right now I'll just do the two 8 byte swaps. I think I will try to add `__builtin_bswap128` in a few days, but it can't be used right now anyways if I understand correctly. Would libc++15 be the earliest revision where this could be changed, assuming clang14 gets `__builtin_bswap128`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114074